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

dev-cmd/bottle: improve :all bottle handling #18350

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

carlocab
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Currently, we silently ignore cases where a formula previously had an
:all bottle but now no longer does.

These are most often due to (in order of likelihood):

  • bottle reproducibility breakage in brew
  • new hard-coded /usr/local references in text files in a bottle

The former is a bug that should be fixed, while the latter can be fixed
trivally with an inreplace.

Let's try to make sure we always do this by making brew bottle error
out so that we can catch these instances as they happen rather than
after the fact.

I haven't encountered any cases where a formula previously had an :all
bottle but no longer does for reasons other than the two outlined above.

If we do encouter those in the future, we can either:

  • update brew bottle to skip this check, perhaps with a new flag
  • delete the formula's old :all bottle before doing brew bottle so
    it doesn't error

Currently, we silently ignore cases where a formula previously had an
`:all` bottle but now no longer does.

These are most often due to (in order of likelihood):
- bottle reproducibility breakage in `brew`
- new hard-coded `/usr/local` references in text files in a bottle

The former is a bug that should be fixed, while the latter can be fixed
trivally with an `inreplace`.

Let's try to make sure we always do this by making `brew bottle` error
out so that we can catch these instances as they happen rather than
after the fact.

I haven't encountered any cases where a formula previously had an `:all`
bottle but no longer does for reasons other than the two outlined above.

If we do encouter those in the future, we can either:
- update `brew bottle` to skip this check, perhaps with a new flag
- delete the formula's old `:all` bottle before doing `brew bottle` so
  it doesn't error
@Bo98
Copy link
Member

Bo98 commented Sep 18, 2024

I haven't encountered any cases where a formula previously had an :all
bottle but no longer does for reasons other than the two outlined above.

When a formula gets a major rewrite in an update, e.g. from a bash script to a native executable. Rare enough not to happen too often but common enough that I've seen it multiple times.

@carlocab
Copy link
Member Author

When a formula gets a major rewrite in an update, e.g. from a bash script to a native executable. Rare enough not to happen too often but common enough that I've seen it multiple times.

Yup, I know it happens, but I haven't seen it personally. Seems simpler to just delete the :all bottle in those cases, but we can add another flag to handle it if it happens.

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.

Makes sense to me, thanks @carlocab!

I think the ideal flow would be a flag for brew bottle, brew test-bot and a CI-no-all-bottle-ok or something label that can be applied to override.

None of this should block this PR so: merging!

@MikeMcQuaid MikeMcQuaid merged commit 21bb3e1 into master Sep 18, 2024
25 checks passed
@MikeMcQuaid MikeMcQuaid deleted the fail-when-no-longer-all-bottle branch September 18, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants