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

extract: Leave a hint in place of the removed bottles block #17590

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kentzo
Copy link
Contributor

@Kentzo Kentzo commented Jun 29, 2024

  • 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?

@@ -133,7 +133,7 @@ def run
result.sub!("class #{class_name} < Formula", "class #{versioned_name} < Formula")

# Remove bottle blocks, as they won't work.
Copy link
Member

Choose a reason for hiding this comment

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

Can the bottles be used? If not, can you please elaborate: what's the value in noting where one can find them?

Also, brew extract can be used on formulae from taps other than homebrew/core, so this may not be accurate.

Copy link
Contributor Author

@Kentzo Kentzo Jun 29, 2024

Choose a reason for hiding this comment

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

Can the bottles be used?

As in re-used from the original formula? No, because after the extraction the name of the formula gets changed from <formula> to <formula>@<version> and thus the URL that Homebrew uses internally gets changed from docker://ghcr.io/homebrew/core/<name>:<version> to docker://ghcr.io/homebrew/core/<name>/<version>:<version>.

what's the value in noting where one can find them

When the intent is to pin formula for a reproducible build, it's important to archive the bottles as well, so one could use Homebrew (an old version, if necessary) with a custom tap and a custom HOMEBREW_BOTTLE_DOMAIN.

As it stands it's hard / impossible to entirely automate the process, e.g. via imaginary brew extract --include-bottles because:

  1. Bottles may not be present
  2. Formula may come from a custom tap

This hint is the next best thing to do as it puts the developer right on track of getting it to work. I'd greatly appreciate it as user.

Perhaps a better solution would be to either document the entire workflow of "taking a snapshot" of a given formula or, at least, document how Homebrew stores and retrieves the bottles.

Also, brew extract can be used on formulae from taps other than homebrew/core, so this may not be accurate.

Right, so I deliberately used the word "might" here :)

Copy link
Member

Choose a reason for hiding this comment

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

Could you just rename the formula to not include the @version part?

As it stands it's hard / impossible to entirely automate the process, e.g. via imaginary brew extract --include-bottles because:

  1. Bottles may not be present
  2. Formula may come from a custom tap

We can simply restrict --include-bottles to homebrew/core formulae, and fail if there are no bottles (or, maybe, still extract the formula and just error on the bottle part). I used to have a custom brew command that just downloaded and extracted bottles here. I stopped using it and I don't think it works anymore, but it could maybe serve as a starting point to implementing something like this.

Copy link
Contributor Author

@Kentzo Kentzo Jun 29, 2024

Choose a reason for hiding this comment

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

@Rylan12 Still there is a problem that Homebrew, being rolling-release package manager, may decide to abandon old bottles.

Instead of --include-bottles I'd rather prever --extract-bottles so I could store them elsewhere and not depend on benevolence of the original formula maintainer.

That being said, while this is under discussion and development I think a hint is a good enough placeholder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

*please also see this discussion: https://github.com/orgs/Homebrew/discussions/5454

@@ -133,7 +133,7 @@ def run
result.sub!("class #{class_name} < Formula", "class #{versioned_name} < Formula")

# Remove bottle blocks, as they won't work.
result.sub!(BOTTLE_BLOCK_REGEX, "")
result.sub!(BOTTLE_BLOCK_REGEX, "# The bottles might be available as oci images at docker://ghcr.io/homebrew/core/#{name}:#{version}")
Copy link
Member

Choose a reason for hiding this comment

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

What if, instead of injecting a comment, we output a warning saying "bottles have been removed because XYZ, but they may still be available at this URL if you'd like to extract them". That way the URL is displayed for easy copy/paste without needing to open the new file to find it, and it also doesn't pollute the new file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This output will be lost with the end of the terminal session while comment stays with the formula forever. And it's easier to script. Perhaps we should do both?

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.

I don't think I support this PR proposal, sorry. I don't think adding a comment here really adds anything and, as mentioned elsewhere, these bottles aren't a drop-in replacement.

@@ -63,5 +63,15 @@
expect(path).to exist
expect(Formulary.factory(path).version).to eq "0.2"
end

it "notes the url where bottles might be found", :integration_test do
Copy link
Member

Choose a reason for hiding this comment

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

Definitely don't need a test for this.

@Kentzo
Copy link
Contributor Author

Kentzo commented Jul 1, 2024

@MikeMcQuaid Would you be interested in the PR adding the --extract-bottles-dir= to support extracting bottles of the homebrew/core formulae via skopeo such that they can be easily re-published later into GitHub Packages of another repo?

@MikeMcQuaid
Copy link
Member

@Kentzo can you explain the workflow of how that would work? Bottles are built for a specific formula name/prefix/keg and cannot be used with another name/prefix/keg.

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.

None yet

4 participants