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.

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

workflows: install svn when brew fetch needs it #201901

Merged
merged 2 commits into from
Dec 24, 2024

Conversation

Moisan
Copy link
Member

@Moisan Moisan commented Dec 20, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

The scheduled.yml workflow is failing on acme and lcs continuously (see #139929) because brew fetch -s needs svn to be installed if the source url is a svn repository.

Note that I'm relatively new to Github actions, I couldn't figure out how to test this change properly.

@Moisan Moisan requested review from MikeMcQuaid and a team as code owners December 20, 2024 12:46
@github-actions github-actions bot added automerge-skip `brew pr-automerge` will skip this pull request workflows PR modifies GitHub Actions workflow files labels Dec 20, 2024
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

I think we should handle this more like the way we handle Homebrew curl instead of trying to parse the output of brew fetch.

@Moisan
Copy link
Member Author

Moisan commented Dec 20, 2024

We would need something like using: :svn to be required on svn url fields in formulae. brew fetch --json=v2 doesn't include any mention of svn currently for formulae like acme and lcs (except svn being in their URL). But I agree that would be much cleaner.

@carlocab
Copy link
Member

You could probably check if svn is a :build dependency. That may have some false positives, but as long as there are no false negatives (and there aren't too many false positives, and there shouldn't be), that seems fine.

Copy link
Member

@ZhongRuoyu ZhongRuoyu left a comment

Choose a reason for hiding this comment

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

$ brew ruby -e 'puts Formula[ARGV.first].deps.any? { |d| d.name == "subversion" && d.implicit? }' acme
true
$ brew ruby -e 'puts Formula[ARGV.first].deps.any? { |d| d.name == "subversion" && d.implicit? }' hello
false

Maybe?

@Moisan
Copy link
Member Author

Moisan commented Dec 20, 2024

brew deps --include-build --direct could also work but it might lead to more false positive than @ZhongRuoyu approach.

@Moisan Moisan force-pushed the workflows_scheduled_fetch_svn branch from c0bbadb to b1d412f Compare December 20, 2024 13:51
Co-authored-by: Carlo Cabrera <[email protected]>
Copy link
Member

@carlocab carlocab 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 to me!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Seems fine for now, would be nice to make this an actual Homebrew command rather than relying on internals here.

@carlocab carlocab added this pull request to the merge queue Dec 24, 2024
@carlocab
Copy link
Member

Seems fine for now, would be nice to make this an actual Homebrew command rather than relying on internals here.

Yes, there's enough non-trivial logic in here that making it a proper brew command makes sense at this point.

Merged via the queue into master with commit 32dafdc Dec 24, 2024
22 checks passed
@carlocab carlocab deleted the workflows_scheduled_fetch_svn branch December 24, 2024 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge-skip `brew pr-automerge` will skip this pull request workflows PR modifies GitHub Actions workflow files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants