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

dwarfs 0.10.1 (new formula) #181569

Merged
merged 2 commits into from
Aug 25, 2024
Merged

Conversation

mhx
Copy link
Contributor

@mhx mhx commented Aug 18, 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>?

@github-actions github-actions bot added new formula PR adds a new formula to Homebrew/homebrew-core boost Boost use is a significant feature of the PR or issue labels Aug 18, 2024
@mhx mhx force-pushed the mhx/add-dwarfs-formula branch 2 times, most recently from 7957711 to 5a731a8 Compare August 18, 2024 11:10
@chenrui333 chenrui333 added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Aug 18, 2024
@mhx mhx force-pushed the mhx/add-dwarfs-formula branch from 5a731a8 to 3919228 Compare August 18, 2024 12:15
@github-actions github-actions bot added the automerge-skip `brew pr-automerge` will skip this pull request label Aug 18, 2024
@mhx
Copy link
Contributor Author

mhx commented Aug 18, 2024

Hi @chenrui333, quick question: I'm seeing

* Dependency 'libarchive' is provided by macOS; please replace 'depends_on' with 'uses_from_macos'.

I didn't see that when running brew audit --new --strict dwarfs locally. Also, I see plenty of formulae that depends_on "libarchive".

The two formulae I've seen that actually uses_from_macos "libarchive" have to jump through a lot of hoops to make it work and even then just get two plain header files. [1] Given that Apple don't consider libarchive public API on macOS, why would Homebrew impose this restriction?

Besides all that, the latest libarchive in Sonoma is v3.5.3 (and older releases will likely be much older than that) and I need a dependency on at least v3.6.0.

Any suggestions?

Thanks! :)

[1] And the way it's done in these formulae is probably not even fully correct; in principle, rather than fetching the sources for a random version of Apple's libarchive, you'd probably have to determine which binary version is installed and then fetch the matching sources tag. I don't think this is a viable solution.

@chenrui333
Copy link
Member

* Dependency 'libarchive' is provided by macOS; please replace 'depends_on' with 'uses_from_macos'.

you can treat that as warming rather than a hard failure.

@mhx
Copy link
Contributor Author

mhx commented Aug 18, 2024

* Dependency 'libarchive' is provided by macOS; please replace 'depends_on' with 'uses_from_macos'.

you can treat that as warming rather than a hard failure.

It looks very much like a failure :)

20240818-151009

I'll give it another try with the libarchive removed from the allowlist.

@mhx mhx force-pushed the mhx/add-dwarfs-formula branch from 8ad0c7b to 4723c5b Compare August 18, 2024 13:11
@github-actions github-actions bot removed the automerge-skip `brew pr-automerge` will skip this pull request label Aug 18, 2024
@mhx mhx marked this pull request as ready for review August 18, 2024 13:12
@mhx
Copy link
Contributor Author

mhx commented Aug 18, 2024

It's actually a hard failure if I remove my change in provided_by_macos_depends_on_allowlist.json: https://github.com/Homebrew/homebrew-core/actions/runs/10440916659/job/28911449754

@mhx
Copy link
Contributor Author

mhx commented Aug 18, 2024

The previous CI run that included the allowlist change was successful: https://github.com/Homebrew/homebrew-core/actions/runs/10440641636

@chenrui333
Copy link
Member

that is audit failure, not hard failure.

@mhx
Copy link
Contributor Author

mhx commented Aug 18, 2024

I'm confused. Why does it fail the CI workflow then? Anything I need to do or is this PR good for review?

@chenrui333
Copy link
Member

I'm confused. Why does it fail the CI workflow then? Anything I need to do or is this PR good for review?

if you search provided by macOS; please replace 'depends_on' with 'uses_from_macos'. you will see this discussions has popped up in the repo many times.

I think the PR is good for review.

@mhx
Copy link
Contributor Author

mhx commented Aug 18, 2024

I'm confused. Why does it fail the CI workflow then? Anything I need to do or is this PR good for review?

if you search provided by macOS; please replace 'depends_on' with 'uses_from_macos'. you will see this discussions has popped up in the repo many times.

I actually did and the first and second hit both suggest adding the package to the allowlist, hence my previous attempt to do just that. :)

@mhx
Copy link
Contributor Author

mhx commented Aug 18, 2024

You obviously have vastly more experience with Homebrew than I do, so this is just my gut feeling and not meant to criticize:

  • It feels wrong to me that libarchive in not in provided_by_macos_depends_on_allowlist.json, given that 37 formulae use the Homebrew version anyway and the two formulae that use the macOS-provided version are significantly more complex because of doing so, and are probably not even fully correct.
  • If also feels wrong to me that it's preferable to have a broken CI workflow rather than adding a package to the allowlist, for lack of alternative options.

If you have any pointers to the rationale behind this it would be highly appreciated. I've done a fair bit of searching, but I'm coming to different conclusions based on what I've found.

@chenrui333
Copy link
Member

  • It feels wrong to me that libarchive in not in provided_by_macos_depends_on_allowlist.json, given that 37 formulae use the Homebrew version anyway and the two formulae that use the macOS-provided version are significantly more complex because of doing so, and are probably not even fully correct.

I dont think it is wrong, we did catch some before

  • If also feels wrong to me that it's preferable to have a broken CI workflow rather than adding a package to the allowlist, for lack of alternative options.

Ideally, we all like green builds, but it is not always the case

@chenrui333
Copy link
Member

I actually did and the first and second hit both suggest adding the package to the allowlist, hence my previous attempt to do just that. :)

I would suggest you take a look at the PRs in this repo, https://github.com/Homebrew/homebrew-core/pull/135198 this ended up not adding the audit_exceptions/provided_by_macos_depends_on_allowlist.json change.

@mhx mhx requested a review from chenrui333 August 20, 2024 20:45
@mhx
Copy link
Contributor Author

mhx commented Aug 24, 2024

So how will this PR make progress now? Is there anything needed from my side?

@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Aug 24, 2024
@mhx mhx force-pushed the mhx/add-dwarfs-formula branch 3 times, most recently from 5b135c5 to 6a54408 Compare August 24, 2024 16:20
@mhx mhx force-pushed the mhx/add-dwarfs-formula branch from 6a54408 to be26b86 Compare August 24, 2024 16:30
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Aug 24, 2024
@mhx mhx requested a review from SMillerDev August 24, 2024 17:29
@mhx mhx force-pushed the mhx/add-dwarfs-formula branch from be26b86 to 37c2d2c Compare August 24, 2024 18:54
@mhx mhx requested a review from SMillerDev August 24, 2024 20:35
@carlocab carlocab added the CI-skip-new-formulae Pass --skip-new to brew test-bot. label Aug 24, 2024
@mhx mhx force-pushed the mhx/add-dwarfs-formula branch from 37c2d2c to ac5ea79 Compare August 25, 2024 07:04
@mhx
Copy link
Contributor Author

mhx commented Aug 25, 2024

Not gonna complain, but why did the build suddenly turn green despite the formula still depending on libarchive on macOS?

@mhx mhx requested a review from carlocab August 25, 2024 07:39
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.

Thanks, just one last suggestion.

@mhx mhx force-pushed the mhx/add-dwarfs-formula branch from ac5ea79 to f9e944d Compare August 25, 2024 08:15
@mhx mhx requested a review from carlocab August 25, 2024 09:04
@carlocab carlocab added this pull request to the merge queue Aug 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 25, 2024
Copy link
Contributor

🤖 An automated task has requested bottles to be published to this PR.

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Aug 25, 2024
@BrewTestBot BrewTestBot enabled auto-merge August 25, 2024 10:23
@BrewTestBot BrewTestBot added this pull request to the merge queue Aug 25, 2024
Merged via the queue into Homebrew:master with commit 0f99695 Aug 25, 2024
15 checks passed
@mhx mhx mentioned this pull request Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
boost Boost use is a significant feature of the PR or issue CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. CI-skip-new-formulae Pass --skip-new to brew test-bot. new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants