Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add CI to detect references #905

Closed
wants to merge 16 commits into from
Closed

feat: add CI to detect references #905

wants to merge 16 commits into from

Conversation

SMillerDev
Copy link
Member

This should tell us when references are no longer in use.

@SMillerDev SMillerDev force-pushed the feat/ci/check_references branch 5 times, most recently from d24b9d0 to 1ced60b Compare February 11, 2024 15:56
@issyl0 issyl0 mentioned this pull request Feb 11, 2024
Closed
@issyl0
Copy link
Member

issyl0 commented Feb 11, 2024

Looks like some of them have .patch or .diff extensions:

Formula/w/webfs.rb:    url "https://github.com/Homebrew/formula-patches/raw/0518a6d1ed821aebf0de4de78e39b57d6e60e296/webfs/patch-ls.c"
Formula/y/yconalyzer.rb:    url "https://raw.githubusercontent.com/Homebrew/formula-patches/85fa66a9/yconalyzer/1.0.4.patch"
Formula/z/zyre.rb:    url "https://raw.githubusercontent.com/Homebrew/formula-patches/03cf8088210822aa2c1ab544ed58ea04c897d9c4/libtool/configure-big_sur.diff"

@issyl0
Better detection of patches - they don't all end in .patch
b642b46 
- Check for which patches are in use in `homebrew-core`, trimming the quotes and only getting the last part of the path.
- Get all the patches in this repository.
- Compare the two and print out the missing ones in the `homebrew-core` repository.
- Add a TODO for deleting the unused patches, it might be nice for this workflow to do that once we know it's legit.
Bo98
Bo98 reviewed Feb 11, 2024
View reviewed changes
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but would rather automated removal rather than yet another manual task for people to check.

.github/workflows/check-reference.yml Outdated Show resolved Hide resolved
.github/workflows/check-reference.yml Outdated Show resolved Hide resolved
.github/workflows/check-reference.yml Outdated Show resolved Hide resolved
@issyl0
Copy link
Member

issyl0 commented Feb 11, 2024 

remote: Permission to Homebrew/formula-patches.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/Homebrew/formula-patches/': The requested URL returned error: 403

@Bo98
Copy link
Member

Bo98 commented Feb 11, 2024 •
edited
Loading 

permissions:
  contents: write

Or push as BrewTestBot using a push token but avoiding BrewTestBot here would be nice.

Bo98
Bo98 reviewed Feb 12, 2024
View reviewed changes
.github/workflows/check-reference.yml Outdated Show resolved Hide resolved
.github/workflows/check-reference.yml Outdated Show resolved Hide resolved
.github/workflows/check-reference.yml Outdated Show resolved Hide resolved
SMillerDev and others added 2 commits February 12, 2024 08:51
@SMillerDev @Bo98
Update check-reference.yml
cb20745 
Co-authored-by: Bo Anderson <[email protected]>
@SMillerDev @Bo98
Update check-reference.yml
e45ec2a 
Co-authored-by: Bo Anderson <[email protected]>
@SMillerDev
Copy link
Member Author

https://github.com/Homebrew/formula-patches/tree/remove-unused-patches exists now. But I guess it also needs to create a PR?

@issyl0
Copy link
Member

issyl0 commented Feb 12, 2024

Or push straight to master? I did the branch to test it. But we could do a PR, but that also requires human intervention to merge.

@SMillerDev
Copy link
Member Author

I'd be slightly uncomfortable pushing directly to master

@SMillerDev
Copy link
Member Author

The base branch requires all commits to be signed.

That's unfortunate, see #908

@Bo98
Copy link
Member

Bo98 commented Mar 12, 2024 •
edited
Loading

Should be able to set up a signing step like we do in all the other repos that commits as BrewTestBot

@SMillerDev
Copy link
Member Author

🥳 #909

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 3, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale label Apr 3, 2024
@SMillerDev
Copy link
Member Author

I'm not sure what to do with this. @Bo98 do you want to rewrite this in ruby and call it with brew ruby?

@github-actions github-actions bot removed the stale label Apr 3, 2024
@MikeMcQuaid
Copy link
Member

@SMillerDev What's the blocker here, not sure I understand?

@SMillerDev
Copy link
Member Author

The generated PR has some comments from Bo, but I'm not sure what to do with them or if we should do something with them.

@MikeMcQuaid
Copy link
Member

The generated PR has some comments from Bo, but I'm not sure what to do with them or if we should do something with them.

@SMillerDev Ok, I saw now, thanks.

Yeh, I think this needs to eliminate the false positives. Fine to not delete things for a V1, not fine to attempt to delete them when still needed.

Could always just add more conditionals to your Bash for now to exclude formulae with any head/OS/arch-specific patches or logic.

@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale label Apr 26, 2024
@github-actions github-actions bot closed this May 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Bo98 Bo98 Bo98 left review comments

@issyl0 issyl0 issyl0 left review comments

@MikeMcQuaid MikeMcQuaid MikeMcQuaid left review comments

Assignees
No one assigned
Labels
outdated stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SMillerDev @issyl0 @Bo98 @MikeMcQuaid