BottleLoader: Use the formula stored in the bottle #3176

sjackman · 2017-09-19T22:57:47Z

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 tests with your changes locally?

When installing a bottle from the local file system or on http, use the formula stored in the bottle.

MikeMcQuaid · 2017-09-22T19:41:02Z

Library/Homebrew/formulary.rb

+      bottle_version = Utils::Bottles.resolve_version @bottle_filename
+      system "tar", "-xf", @bottle_filename, "-C", HOMEBREW_CACHE_FORMULA,
+        "--strip-components", "3", "#{name}/#{bottle_version}/.brew/#{name}.rb",
+        err: :close


It would be good to avoid creating/deleting/writing files in the formulary loader. How about using something like tar -O to read it from stdout?

Good suggestion, Mike. Done.

MikeMcQuaid · 2017-09-23T15:33:25Z

Library/Homebrew/formulary.rb

+      bottle_version = Utils::Bottles.resolve_version @bottle_filename
+      path = "#{name}/#{bottle_version}/.brew/#{name}.rb"
+      contents = Utils.popen_read "tar", "-xO", "-f", @bottle_filename,
+        "--strip-components", "3", path, err: :close


I don't want err: :close to be going throughout the codebase when they are no-ops for most users and when, in this case, the error should never be hit. I'm tempted to say rather than falling back this should raise an exception instead. Thoughts?

Agreed. I'll change it.

MikeMcQuaid · 2017-09-23T15:33:41Z

Library/Homebrew/formulary.rb

+      formula = if $CHILD_STATUS.success?
+        Formulary.from_contents name, HOMEBREW_CELLAR/path, contents, spec
+      else
+        super


If this stays as-is: worth printing a Warning message in this case?

I've added a warning.

MikeMcQuaid · 2017-09-24T17:08:51Z

Library/Homebrew/formulary.rb

+      formula = if $CHILD_STATUS.success?
+        Formulary.from_contents name, HOMEBREW_CELLAR/path, contents, spec
+      else
+        opoo "This bottle does not contain a formula: #{path}"


I think this should probably just fail rather than have inconsistent behaviour. If you're loading bottles more than a year old you're going to have other problems.

Agreed. Done.

sjackman · 2017-09-25T06:37:45Z

Library/Homebrew/formulary.rb

@@ -122,14 +122,14 @@ def initialize(bottle_name)
      super name, Formulary.path(full_name)
    end

-    def get_formula(spec, alias_path: nil)
-      formula = super
+    def get_formula(spec, alias_path: nil) # rubocop:disable Lint/UnusedMethodArgument


Is # rubocop:disable Lint/UnusedMethodArgument the best way to avoid this brew style error?

W:125: 27: Unused method argument - alias_path.

See rubocop/rubocop#3130

Would you prefer…

raise "Unexpected non-nil parameter alias_path" unless alias_path.nil?

I think def get_formula(spec, **) should work.

Yes, that works, but it does alter the behavior of the method for style, in that get_formula(spec, monkey: :banana) no longer fails. rubocop/rubocop#3130 (comment)
My minor style preference is raise … unless nil?. I'm fine with ** though.

sjackman · 2017-09-25T16:04:36Z

I have a failed test to address. I'll get back to you with that later today.

  1) Formulary::factory returns a Formula when given a bottle
     Failure/Error: formula = subject.factory(bottle)
     
     BottleFormulaUnavailableError:
       This bottle does not contain the formula file:
         testball_bottle/0.1/.brew/testball_bottle.rb

MikeMcQuaid · 2017-09-25T17:41:00Z

Library/Homebrew/exceptions.rb

-      Bottle version mismatch
-      Bottle: #{bottle_file} (#{bottle_version})
-      Formula: #{formula.full_name} (#{formula_version})
+      This bottle does not contain the formula file:


How about outputting the bottle file and expected formula path within it?

MikeMcQuaid · 2017-09-25T17:41:24Z

Library/Homebrew/formulary.rb

-    def get_formula(spec, alias_path: nil)
-      formula = super
+    def get_formula(spec, alias_path: nil) # rubocop:disable Lint/UnusedMethodArgument
+      bottle_version = Utils::Bottles.resolve_version @bottle_filename


Can this fail?

If the bottle does not contain a file matching the regex .+/.+/INSTALL_RECEIPT.json, then yes, it could fail. It currently throws an exception:

Error: undefined method `split' for nil:NilClass /Users/sjackman/.homebrew/Library/Homebrew/utils/bottles.rb:40:in `resolve_formula_names'

I've added

--- a/Library/Homebrew/utils/bottles.rb +++ b/Library/Homebrew/utils/bottles.rb @@ -29,9 +29,11 @@ module Utils end def receipt_path(bottle_file) - Utils.popen_read("tar", "-tzf", bottle_file).lines.map(&:chomp).find do |line| + path = Utils.popen_read("tar", "-tzf", bottle_file).lines.map(&:chomp).find do |line| line =~ %r{.+/.+/INSTALL_RECEIPT.json} end + raise "This bottle does not contain the file INSTALL_RECEIPT.json: #{bottle_file}" if path.nil? + path end def resolve_formula_names(bottle_file)

MikeMcQuaid · 2017-09-25T17:42:06Z

Library/Homebrew/formulary.rb

+      contents = Utils.popen_read "tar", "-xO", "-f", @bottle_filename, "--strip-components=3", formula_path
+      raise BottleFormulaUnavailableError, formula_path unless $CHILD_STATUS.success?
+      formula = Formulary.from_contents name, HOMEBREW_CELLAR/formula_path, contents, spec
+      formula.bottle_specification.sha256 @bottle_filename.sha256 => Utils::Bottles.tag


What happens without this, out of interest? I'm 👍 on it, just curious.

Because the formula stored in the bottle does not have a bottle block, the bottle method returns nil, and causes the error. In principle that error would have also been possible before if the formula in the repo did not contain a bottle block, which is uncommon.

==> Pouring the cached bottle Error: undefined method `compatible_cellar?' for nil:NilClass /Users/sjackman/.homebrew/Library/Homebrew/formula_installer.rb:104:in `pour_bottle?'

I'll add a commit to address this issue.

MikeMcQuaid · 2017-09-25T19:11:08Z

Library/Homebrew/formula_installer.rb

@@ -101,7 +101,7 @@ def pour_bottle?(install_bottle_options = { warn: false })
      return false
    end

-    unless bottle.compatible_cellar?
+    if bottle && !bottle.compatible_cellar?


unless bottle&.compatible_cellar?

I didn't know about the &. operator! Fantastic!

Will be enforced by brew style from Homebrew/homebrew-portable-ruby#50. Wasn't supported in Ruby 2.0.

formula.bottle is nil when bottle.compatible_cellar? is false.
Using formula.bottle_specification.compatible_cellar? rather
than formula.bottle.compatible_cellar? avoids this problem

MikeMcQuaid · 2017-09-25T19:12:04Z

Library/Homebrew/utils/bottles.rb

          line =~ %r{.+/.+/INSTALL_RECEIPT.json}
        end
+        raise "This bottle does not contain the file INSTALL_RECEIPT.json: #{bottle_file}" if path.nil?


unless path

MikeMcQuaid

Looks good. Let me know when you've tested this thoroughly locally and will merge. Thanks!

sjackman · 2017-09-26T05:54:11Z

I found one odd use case: brew install -s local_bottle.tar.gz failed when copying the formula to .brew/NAME.rb, because the formula is inside the bottle and not stored anywhere on disk. I've fixed this issue with the most recent commit, 46fa99c3d68c0e9fc37203bdaa37f6a45bb7ef30.

sjackman · 2017-09-26T06:35:27Z

Do you think it would be useful to add a contents method to the Formula class, that returns the Ruby code of the formula?

MikeMcQuaid · 2017-09-26T07:35:02Z

@sjackman If there's 3 or more locations it would be used: yep, otherwise: nope.

ilovezfs · 2017-09-26T08:28:39Z

Masking the formula that is in the git repository in favor of one buried in a binary and, in particular, allowing that to override the checksum, seems like a security risk to me.

MikeMcQuaid · 2017-09-26T08:33:07Z

Masking the formula that is in the git repository in favor of one buried in a binary and, in particular, allowing that to override the checksum, seems like a security risk to me.

I think the checksum should be able to be set if it's unset if that's needed for internal code but it shouldn't be able to be overridden, I agree. I wonder if it's worth reading the version from the both formulae and using the one in Git if the version/revision/pkg_version match (as otherwise we are unlikely to have old bottles remain working after e.g. major post_install changes). Alternatively, the best middle ground might be only using the post_install from the bottled formula as that's the only bit that affects bottles at all.

ilovezfs · 2017-09-26T08:36:55Z

If we patch a critical CVE that would still let someone blithely brew install ./badversion since the versions wouldn't be the same.

MikeMcQuaid · 2017-09-26T09:30:45Z

A thought: a warning should be output when loading a formula that exists in the tap from the tab at a newer version. If we’re aware an older version is being installed (which we can be): we should warn.

sjackman · 2017-09-26T15:03:02Z

Sure. I can do that.

sjackman · 2017-09-26T17:16:11Z

My most recent commit displays a warning when downgrading a formula.

sjackman · 2017-09-27T16:43:51Z

I think the checksum should be able to be set if it's unset if that's needed for internal code

Adding the sha256 to the bottle spec is no longer necessary thanks to fixing up the logic in pour_bottle? not to freak out when formula.bottle is nil. I've removed the line that sets the sha256 of the bottle.

MikeMcQuaid · 2017-09-27T19:46:16Z

Library/Homebrew/formula_installer.rb

@@ -101,6 +100,7 @@ def pour_bottle?(install_bottle_options = { warn: false })
      return false
    end

+    bottle = formula.bottle_specification


Why is this needed below, out of interest, and bottle_specification rather than bottle?

formula.bottle returns nil when compatible_cellar? is false.
See https://github.com/Homebrew/brew/blob/master/Library/Homebrew/formula.rb#L330
and https://github.com/Homebrew/brew/blob/master/Library/Homebrew/software_spec.rb#L93

We could use formula.bottled? rather than formula.bottle_specification.compatible_cellar?. Do you have a preference?

formula.bottled? would be preferable, yeh.

MikeMcQuaid · 2017-09-27T19:47:01Z

Library/Homebrew/formula_installer.rb

@@ -235,6 +235,10 @@ def install
      end
      raise CannotInstallFormulaError, message
    end
+    if formula.opt_prefix.directory? &&
+       formula.pkg_version < (v = Keg.new(formula.opt_prefix.resolved_path).version)
+      opoo "Downgrading #{formula.name} from #{v} to #{formula.pkg_version}."


Sorry, rather than downgrading here would be good to check the tab in the bottle and see if the formula file exists and that one has a newer version.

Ah, got it. Sorry I misunderstood. I've updated the warning.

MikeMcQuaid · 2017-09-27T19:47:42Z

Library/Homebrew/test/support/fixtures/testball_bottle.rb

@@ -6,7 +6,7 @@ def initialize(name = "testball_bottle", path = Pathname.new(__FILE__).expand_pa
      stable.bottle do
        cellar :any_skip_relocation
        root_url "file://#{TEST_FIXTURE_DIR}/bottles"
-        sha256 "9abc8ce779067e26556002c4ca6b9427b9874d25f0cafa7028e05b5c5c410cb4" => Utils::Bottles.tag
+        sha256 "d48bbbe583dcfbfa608579724fc6f0328b3cd316935c6ea22f134610aaf2952f" => Utils::Bottles.tag


Presumably: adding the formula?

MikeMcQuaid · 2017-09-28T19:05:53Z

Library/Homebrew/formula_installer.rb

+    # Warn if a more recent version of this formula is available in the tap.
+    begin
+      if formula.pkg_version < (v = Formulary.factory(formula.full_name).pkg_version)
+        opoo "#{formula.name} #{v} is available and more recent than version #{formula.pkg_version}."


Use formula.full_name here? @ilovezfs, does this look OK?

formula.bottle is nil when bottle.compatible_cellar? is false. Use formula.bottle_specification.compatible_cellar? rather than formula.bottle.compatible_cellar?.

Update the SHA-256.

Factor Utils::Bottles.formula_contents out of BottleLoader.

Warn if a more recent version of this formula is available in the tap.

MikeMcQuaid · 2017-09-29T12:59:49Z

Thanks again @sjackman!

sjackman · 2017-09-29T15:42:53Z

Woo hoo! Thanks for merging, Mike!

MikeMcQuaid requested changes Sep 22, 2017

View reviewed changes

sjackman force-pushed the bottle-formula branch 2 times, most recently from 3883eea to 1320d39 Compare September 22, 2017 22:43

MikeMcQuaid reviewed Sep 23, 2017

View reviewed changes

sjackman force-pushed the bottle-formula branch 3 times, most recently from 4b546eb to 2654b9f Compare September 24, 2017 15:37

MikeMcQuaid reviewed Sep 24, 2017

View reviewed changes

sjackman force-pushed the bottle-formula branch 2 times, most recently from 6e0516c to 622831b Compare September 25, 2017 06:36

sjackman commented Sep 25, 2017

View reviewed changes

MikeMcQuaid reviewed Sep 25, 2017

View reviewed changes

sjackman force-pushed the bottle-formula branch from 622831b to afac925 Compare September 25, 2017 18:41

MikeMcQuaid reviewed Sep 25, 2017

View reviewed changes

sjackman force-pushed the bottle-formula branch 2 times, most recently from 9d1f214 to aa98aca Compare September 25, 2017 23:16

MikeMcQuaid approved these changes Sep 26, 2017

View reviewed changes

sjackman force-pushed the bottle-formula branch from 731b1b1 to 46fa99c Compare September 26, 2017 05:56

receipt_path: Ensure the bottle contains INSTALL_RECEIPT.json

1e57445

sjackman force-pushed the bottle-formula branch from ffef94e to b67c670 Compare September 27, 2017 16:42

MikeMcQuaid reviewed Sep 27, 2017

View reviewed changes

sjackman force-pushed the bottle-formula branch 2 times, most recently from 7fc5ca9 to 4d3dbae Compare September 28, 2017 18:48

MikeMcQuaid reviewed Sep 28, 2017

View reviewed changes

sjackman force-pushed the bottle-formula branch from 4d3dbae to ab2ba27 Compare September 28, 2017 19:15

sjackman added 5 commits September 28, 2017 12:29

pour_bottle?: Fix when formula.bottle is nil

3ebf282

formula.bottle is nil when bottle.compatible_cellar? is false. Use formula.bottle_specification.compatible_cellar? rather than formula.bottle.compatible_cellar?.

testball_bottle-0.1: Add the formula to the bottle

9642ec7

Update the SHA-256.

BottleLoader: Use the formula stored in the bottle

3ed832d

Fix installing a local bottle from source

2e77de3

Factor Utils::Bottles.formula_contents out of BottleLoader.

FormulaInstaller: Warn when tap version is newer

c19cc70

Warn if a more recent version of this formula is available in the tap.

sjackman force-pushed the bottle-formula branch from ab2ba27 to c19cc70 Compare September 28, 2017 19:29

MikeMcQuaid approved these changes Sep 29, 2017

View reviewed changes

MikeMcQuaid merged commit 296a441 into Homebrew:master Sep 29, 2017

sjackman deleted the bottle-formula branch September 29, 2017 15:42

This was referenced Oct 1, 2017

Revert "BottleLoader: Use the formula stored in the bottle" #3249

Closed

python3 3.7.0a1 (devel) Homebrew/homebrew-core#18776

Closed

Source build can fail when pour_bottle? do is unsatisfied #3251

Closed

MikeMcQuaid mentioned this pull request Oct 4, 2017

formulary: handle unreadable bottle formula. #3263

Merged

Homebrew locked and limited conversation to collaborators May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BottleLoader: Use the formula stored in the bottle #3176

BottleLoader: Use the formula stored in the bottle #3176

BottleLoader: Use the formula stored in the bottle #3176

BottleLoader: Use the formula stored in the bottle #3176

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment