Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Honor built_bottle=false when upgrading formulae. #11777

Closed
wants to merge 1 commit into from

3 participants

@nicolasdespres

This commit fixes #11552.

@mikemcquaid
Owner

I'm not happy with adding this as-is because it will break new bottles when we add them. We should instead detect if options were used on upgrade and disable the bottle installation. I'll have a look.

@nicolasdespres

Thanks for your time. Honestly, I did not really understand your first sentence. I don't have a lot of insight with the bottle system. I have tested brew install and brew upgrade and they both seems to work properly. brew install still take the bottle by default when one is provided by the formulae.

@mikemcquaid
Owner

If we introduce a bottle for e.g. MySQL then on upgrade with this patch no-one will get the bottle (unless they do a manual uninstall/install).

@nicolasdespres

Ok, I get it. Thanks for the explanation. I think it is hard to know what the users actually want. For instance, I don't use any option for cmake but I don't use the bottle and when I upgrade I don't want to get the bottle neither. So,

Actually, I have the same problem with my brew-dump external command since in the meantime a bottle may have been added to the formulae. I think we should at least be preventive and stick as much as possible to what the user previously decided.

Also the opposite problem may be broken by my patch. What if a formulae used to have a bottle and the new one no longer provide one. I haven't tested this case.

Maybe we can decide to upgrade using a bottle if f.have_bottle? and tab.used_options.empty?

@nicolasdespres

I compute this condition with the following table:

tab.used_options.size > 0 = 0 | 1 | 0 | 1 | 0 | 1 | 0 | 1 |
tab.built_bottle          = 0 | 0 | 1 | 1 | 0 | 0 | 1 | 1 |
f.have_bottle?            = 0 | 0 | 0 | 0 | 1 | 1 | 1 | 1 |
upgrade with bottle       = 0 | 0 | 0 | 0 | 1 | 0 | 1 | 0 |

I'm not sure about the 5th column since this is case I am facing with cmake.

@mikemcquaid
Owner

Yep, if f.have_bottle? and used_options.empty? then that's enough to use a bottle.

@nicolasdespres

If and only if bottle are built without any special options users don't want. I don't know the current policy for bottle building but I guess this assumption is right. I'll try to push a new proposal ASAP.

Thanks.

@mikemcquaid
Owner

Bottles are never built with special options but may be built with --universal in future (that will require code changes though so don't worry about it).

@mikemcquaid
Owner

This looks pretty reasonable now. FYI I don't get notified when you push a new version so leave a comment in future, thanks!

@mikemcquaid
Owner

@jacknagel @adamv @mistydemeo @Sharpie @mxcl Can someone look at this? I don't have the bandwidth right now :(

@jacknagel
Owner

I'm curious as to why pushing --build-from-source onto ARGV in FormulaInstaller#build does anything; IIRC bottles are installed by FormulaInstaller directly and not by the forked build script.

@nicolasdespres

@mikemcquaid Thanks for the information. I did not know that.
@jacknagel If I remember well I had to add this line because other wise the bottle package is downloaded, extracted and then the build start on it and obviously fail. Maybe there is a better fix.

@nicolasdespres

@jacknagel Withtout that we have to patch Formula.fetch.

@jacknagel
Owner

I am doing some work to refactor formula specs that should make that "just work", so maybe add a FIXME comment or something so I remember to remove it.

@nicolasdespres

FIXME added.

@mikemcquaid
Owner

Looks good to me. @jacknagel ?

@jacknagel
Owner

Will take a look this evening.

@jacknagel
Owner

I think the logic is right, but I question this method:

def upgrade_with_bottle?
  install_bottle?(@f) and @tab.used_options.empty?
end

I don't think checking it is necessary here:

args << '--build-from-source' unless upgrade_with_bottle?

IOW, if we are in FormulaInstaller#build, upgrade_with_bottle? should never be true.

and the other place where it is used, in upgrade.rb:

installer.install_bottle = installer.upgrade_with_bottle?

I think we should just be explicit, i.e.

installer.install_bottle = install_bottle?(f) and tab.used_options.empty?

Please correct me if I am wrong on any point.

@nicolasdespres

I kind of agree with your reasoning but:

  • @tab.used_options.empty? may be true. Actually, it is true most of the time, except for the case I want to fix.
  • install_bottle?(@f) may be true
    • if --install-bottle is passed: ok this is a special case.
    • not ARGV.build_from_source? is true
    • bottle_current?(f) returned value may change along the lifetime of a formulae.

So, we could only keep @tab.used_options.empty? to check whether to add --build-from-source but since a formulae may have provided a bottle and it no longer provides and reversely, we also have to keep at least bottle_current?(f) in the condition.

I hope, I am clear in my reasoning.

@jacknagel
Owner

But none of that matters by the time we're inside FormulaInstaller#build. Once we're there, we can _only_ build from source.

@nicolasdespres
    if install_bottle
      pour
    else
      build
      clean
    end

Said this way, that's remember me the surprise I had to have to patch in there. So I agree with you, according to the code block above you can add --build-from-source without any condition.

@jacknagel
Owner

Right. So I think the patch should look like this:

diff --git a/Library/Homebrew/cmd/upgrade.rb b/Library/Homebrew/cmd/upgrade.rb
index d9c1309..c2ba0dc 100644
--- a/Library/Homebrew/cmd/upgrade.rb
+++ b/Library/Homebrew/cmd/upgrade.rb
@@ -54,6 +54,7 @@ module Homebrew extend self

     installer = FormulaInstaller.new(f, tab)
     installer.show_header = false
+    installer.install_bottle = install_bottle(f) and tab.used_options.empty?

     oh1 "Upgrading #{f.name}"

diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb
index 3f48e2d..db893e2 100644
--- a/Library/Homebrew/formula_installer.rb
+++ b/Library/Homebrew/formula_installer.rb
@@ -182,7 +182,12 @@ class FormulaInstaller

     args = ARGV.clone
     unless args.include? '--fresh'
-      args.concat tab.used_options unless tab.nil?
+      unless tab.nil?
+        args.concat tab.used_options
+        # FIXME: enforce the download of the non-bottled package
+        # in the spawned Ruby process.
+        args << '--build-from-source'
+      end
       args.uniq! # Just in case some dupes were added
     end
@nicolasdespres

I agree. I'll push a new version tomorrow.

@nicolasdespres

I have tested the patch with the upgrade of git 1.7.10.1 which is not a very good test case since this formulae does not provide a bottle but at least it stresses the code and show off that install_bottle(f) required a trailing question mark, so I think it works even if some typo may remain elsewhere...

@mikemcquaid
Owner

A small bottle to test with would be something like gettext.

@nicolasdespres

Ok, but what it is the best protocol to test such a thing? Should I look for a old version of the gettext formula using $ git log -p -- Library/Formula/gettext.rb, checkout this version on a branch, reinstall gettext, checkout master, and test brew upgrade gettext? I have quickly search the documentation for the developer but I did not find anything. Do you guys have special tools such as a sandbox where you can play without disturbing your /usr/local/. Up to now, I have been testing brew upgrade just by opportunity because rebasing my local master with the upstream did trigger some formulae to upgrade.

@mikemcquaid
Owner

I just use my /usr/local. If I want to be safe I sometimes move my Cellar to Cellar2.

@jacknagel
Owner

When I need to fake an upgrade, I just rename an existing keg (to anything). Since we don't remove old kegs during upgrades, I can just rename it back when I'm done if I want the old one.

@nicolasdespres

Thank you all for you tips. I have tested the patch it is seems to work fine. I have just rebased it on top of master.

@jacknagel
Owner

Alright, merged. Thanks for your patience.

@jacknagel jacknagel closed this pull request from a commit
@nicolasdespres nicolasdespres Honor build options and bottles when upgrading
Fixes #11552.
Closes #11777.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>
1c74a94
@jacknagel jacknagel closed this in 1c74a94
@Sharpie Sharpie referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@mikemcquaid
Owner

Thanks @polrop!

@rohansingh rohansingh referenced this pull request from a commit in rohansingh/homebrew
@nicolasdespres nicolasdespres Honor build options and bottles when upgrading
Fixes #11552.
Closes #11777.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>
373f702
@nicolasdespres

You are welcome!

@Sharpie Sharpie referenced this pull request from a commit in Sharpie/homebrew
@nicolasdespres nicolasdespres Honor build options and bottles when upgrading
Fixes #11552.
Closes #11777.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>
0d341b0
@zhangcheng zhangcheng referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@snakeyroc3 snakeyroc3 referenced this pull request from a commit in snakeyroc3/homebrew
@nicolasdespres nicolasdespres Honor build options and bottles when upgrading
Fixes #11552.
Closes #11777.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>
2e42048
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 5, 2012
  1. @nicolasdespres
This page is out of date. Refresh to see the latest.
View
1  Library/Homebrew/cmd/upgrade.rb
@@ -54,6 +54,7 @@ def upgrade_formula f
installer = FormulaInstaller.new(f, tab)
installer.show_header = false
+ installer.install_bottle = install_bottle?(f) and tab.used_options.empty?
oh1 "Upgrading #{f.name}"
View
7 Library/Homebrew/formula_installer.rb
@@ -182,7 +182,12 @@ def build
args = ARGV.clone
unless args.include? '--fresh'
- args.concat tab.used_options unless tab.nil?
+ unless tab.nil?
+ args.concat tab.used_options
+ # FIXME: enforce the download of the non-bottled package
+ # in the spawned Ruby process.
+ args << '--build-from-source'
+ end
args.uniq! # Just in case some dupes were added
end
Something went wrong with that request. Please try again.