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

BottleLoader: Use the formula stored in the bottle #3176

Merged
merged 6 commits into from Sep 29, 2017

Conversation

Projects
None yet
3 participants
@sjackman
Copy link
Member

sjackman commented Sep 19, 2017

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

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 22, 2017

Member

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?

This comment has been minimized.

@sjackman

sjackman Sep 22, 2017

Author Member

Good suggestion, Mike. Done.

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

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 23, 2017

Member

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?

This comment has been minimized.

@sjackman

sjackman Sep 24, 2017

Author Member

Agreed. I'll change it.

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 23, 2017

Member

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

This comment has been minimized.

@sjackman

sjackman Sep 24, 2017

Author Member

I've added a warning.

@sjackman sjackman force-pushed the sjackman:bottle-formula branch 3 times, most recently from 4b546eb to 2654b9f Sep 24, 2017

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 24, 2017

Member

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.

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

Agreed. Done.

@sjackman sjackman force-pushed the sjackman:bottle-formula branch 2 times, most recently from 6e0516c to 622831b Sep 25, 2017

@@ -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

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

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

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

See rubocop-hq/rubocop#3130

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

Would you prefer…

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2017

Member

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

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

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-hq/rubocop#3130 (comment)
My minor style preference is raise … unless nil?. I'm fine with ** though.

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

Done.

@sjackman

This comment has been minimized.

Copy link
Member Author

sjackman commented Sep 25, 2017

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
Bottle version mismatch
Bottle: #{bottle_file} (#{bottle_version})
Formula: #{formula.full_name} (#{formula_version})
This bottle does not contain the formula file:

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2017

Member

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

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

Done.

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2017

Member

Can this fail?

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

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'

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

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)
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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2017

Member

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

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

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.

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

Done.

@sjackman sjackman force-pushed the sjackman:bottle-formula branch from 622831b to afac925 Sep 25, 2017

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

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2017

Member

unless bottle&.compatible_cellar?

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2017

Member

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

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

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

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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2017

Member

unless path

This comment has been minimized.

@sjackman

sjackman Sep 25, 2017

Author Member

Done.

@sjackman sjackman force-pushed the sjackman:bottle-formula branch 2 times, most recently from 9d1f214 to aa98aca Sep 25, 2017

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

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

@sjackman

This comment has been minimized.

Copy link
Member Author

sjackman commented Sep 26, 2017

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, 46fa99c.

@sjackman sjackman force-pushed the sjackman:bottle-formula branch from 731b1b1 to 46fa99c Sep 26, 2017

@sjackman

This comment has been minimized.

Copy link
Member Author

sjackman commented Sep 26, 2017

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 26, 2017

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

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 26, 2017

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 26, 2017

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

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 26, 2017

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 26, 2017

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

This comment has been minimized.

Copy link
Member Author

sjackman commented Sep 26, 2017

Sure. I can do that.

@sjackman

This comment has been minimized.

Copy link
Member Author

sjackman commented Sep 26, 2017

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

@sjackman sjackman force-pushed the sjackman:bottle-formula branch from ffef94e to b67c670 Sep 27, 2017

@sjackman

This comment has been minimized.

Copy link
Member Author

sjackman commented Sep 27, 2017

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.

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

bottle = formula.bottle_specification

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 27, 2017

Member

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

This comment has been minimized.

@sjackman

sjackman Sep 28, 2017

Author Member

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?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 28, 2017

Member

formula.bottled? would be preferable, yeh.

This comment has been minimized.

@sjackman

sjackman Sep 28, 2017

Author Member

Done.

@@ -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}."

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 27, 2017

Member

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.

This comment has been minimized.

@sjackman

sjackman Sep 28, 2017

Author Member

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

@@ -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

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 27, 2017

Member

Presumably: adding the formula?

This comment has been minimized.

@sjackman

sjackman Sep 28, 2017

Author Member

Yep

@sjackman sjackman force-pushed the sjackman:bottle-formula branch 2 times, most recently from 7fc5ca9 to 4d3dbae Sep 28, 2017

# 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}."

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 28, 2017

Member

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

This comment has been minimized.

@sjackman

sjackman Sep 28, 2017

Author Member

Done.

@sjackman sjackman force-pushed the sjackman:bottle-formula branch from 4d3dbae to ab2ba27 Sep 28, 2017

sjackman added some commits Sep 25, 2017

pour_bottle?: Fix when formula.bottle is nil
formula.bottle is nil when bottle.compatible_cellar? is false.
Use formula.bottle_specification.compatible_cellar? rather
than formula.bottle.compatible_cellar?.
Fix installing a local bottle from source
Factor Utils::Bottles.formula_contents out of BottleLoader.
FormulaInstaller: Warn when tap version is newer
Warn if a more recent version of this formula is available in the tap.

@sjackman sjackman force-pushed the sjackman:bottle-formula branch from ab2ba27 to c19cc70 Sep 28, 2017

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 29, 2017

Thanks again @sjackman!

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

3 checks passed

codecov/patch 91.66% of diff hit (target 67.08%)
Details
codecov/project 67.09% (+<.01%) compared to cb139ca
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sjackman sjackman deleted the sjackman:bottle-formula branch Sep 29, 2017

@sjackman

This comment has been minimized.

Copy link
Member Author

sjackman commented Sep 29, 2017

Woo hoo! Thanks for merging, Mike!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.