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

pour_bottle?: Pour local bottles without sha256 #3105

Merged
merged 2 commits into from Aug 31, 2017

Conversation

Projects
None yet
2 participants
@sjackman
Copy link
Contributor

sjackman commented Aug 29, 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?

Pouring a local bottle for a formula without a bottle sha256
in the formula for that OS would unexpectedly install from
source instead.

@@ -85,7 +85,7 @@ def pour_bottle?(install_bottle_options = { warn: false })
return false if @pour_failed

bottle = formula.bottle
return false unless bottle
return false unless bottle || formula.local_bottle_path

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 29, 2017

Member

return true if formula.local_bottle_path is on line 94 already so it feels weird to duplicate it here. Can this logic be moved around instead so it's only checked once?

This comment has been minimized.

@sjackman

sjackman Aug 30, 2017

Contributor

I've removed return true if formula.local_bottle_path. Pouring a local bottle should still check that that bottle is compatible. --force-bottle may be used to pour a local bottle that is incompatible, as it is for remote bottles.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 30, 2017

Member

Ok, cool. Can you make this an if instead; unless with multiple conditions are hard to parse. After that should be 👍 to 🚢

This comment has been minimized.

@sjackman

sjackman Aug 30, 2017

Contributor

Done. Thanks for the feedback, Mike.

@sjackman sjackman force-pushed the sjackman:pour_local_bottle branch from 96821e5 to 77a59ba Aug 30, 2017

sjackman added some commits Aug 30, 2017

pour_bottle?: Ensure local bottles are compatible
Don't ignore f.pour_bottle? and compatible_cellar? when pouring
a local bottle. --force-bottle may be used to pour a local
bottle that is incompatible, as it is for remote bottles.
pour_bottle?: Pour local bottles without sha256
Pouring a local bottle for a formula without a bottle sha256
in the formula for that OS would unexpectedly install from
source instead.

@sjackman sjackman force-pushed the sjackman:pour_local_bottle branch from 77a59ba to 4cfd333 Aug 30, 2017

@MikeMcQuaid MikeMcQuaid merged commit b2cd52d into Homebrew:master Aug 31, 2017

3 checks passed

codecov/patch 100% of diff hit (target 50.49%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +49.5% compared to 156bca7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 31, 2017

Thanks again @sjackman!

@sjackman sjackman deleted the sjackman:pour_local_bottle branch Aug 31, 2017

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented Aug 31, 2017

My pleasure. Thanks for the feedback, and thanks for merging, Mike.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

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