This repository has been archived by the owner. It is now read-only.

Honor built_bottle=false when upgrading formulae. #11777

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@nicolasdespres
Contributor

nicolasdespres commented Apr 20, 2012

This commit fixes #11552.

@MikeMcQuaid

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid Apr 20, 2012

Member

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.

Member

MikeMcQuaid commented Apr 20, 2012

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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres Apr 21, 2012

Contributor

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.

Contributor

nicolasdespres commented Apr 21, 2012

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

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid Apr 21, 2012

Member

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

Member

MikeMcQuaid commented Apr 21, 2012

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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres Apr 21, 2012

Contributor

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?

Contributor

nicolasdespres commented Apr 21, 2012

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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres Apr 21, 2012

Contributor

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.

Contributor

nicolasdespres commented Apr 21, 2012

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

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid Apr 22, 2012

Member

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

Member

MikeMcQuaid commented Apr 22, 2012

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

@nicolasdespres

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres Apr 22, 2012

Contributor

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.

Contributor

nicolasdespres commented Apr 22, 2012

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

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid Apr 22, 2012

Member

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

Member

MikeMcQuaid commented Apr 22, 2012

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

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid Apr 28, 2012

Member

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

Member

MikeMcQuaid commented Apr 28, 2012

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

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid Apr 28, 2012

Member

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

Member

MikeMcQuaid commented Apr 28, 2012

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

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel Apr 28, 2012

Contributor

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.

Contributor

jacknagel commented Apr 28, 2012

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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres Apr 28, 2012

Contributor

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

Contributor

nicolasdespres commented Apr 28, 2012

@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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres Apr 28, 2012

Contributor

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

Contributor

nicolasdespres commented Apr 28, 2012

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

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel Apr 28, 2012

Contributor

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.

Contributor

jacknagel commented Apr 28, 2012

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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres Apr 30, 2012

Contributor

FIXME added.

Contributor

nicolasdespres commented Apr 30, 2012

FIXME added.

@MikeMcQuaid

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid May 2, 2012

Member

Looks good to me. @jacknagel ?

Member

MikeMcQuaid commented May 2, 2012

Looks good to me. @jacknagel ?

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel May 2, 2012

Contributor

Will take a look this evening.

Contributor

jacknagel commented May 2, 2012

Will take a look this evening.

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel May 3, 2012

Contributor

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.

Contributor

jacknagel commented May 3, 2012

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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres May 3, 2012

Contributor

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.

Contributor

nicolasdespres commented May 3, 2012

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

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel May 3, 2012

Contributor

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

Contributor

jacknagel commented May 3, 2012

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

@nicolasdespres

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres May 3, 2012

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

Contributor

nicolasdespres commented May 3, 2012

    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

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel May 3, 2012

Contributor

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
Contributor

jacknagel commented May 3, 2012

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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres May 4, 2012

Contributor

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

Contributor

nicolasdespres commented May 4, 2012

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

@nicolasdespres

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres May 4, 2012

Contributor

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

Contributor

nicolasdespres commented May 4, 2012

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

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid May 4, 2012

Member

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

Member

MikeMcQuaid commented May 4, 2012

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

@nicolasdespres

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres May 4, 2012

Contributor

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.

Contributor

nicolasdespres commented May 4, 2012

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

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid May 4, 2012

Member

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

Member

MikeMcQuaid commented May 4, 2012

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

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel May 4, 2012

Contributor

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.

Contributor

jacknagel commented May 4, 2012

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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres May 5, 2012

Contributor

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.

Contributor

nicolasdespres commented May 5, 2012

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

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel May 5, 2012

Contributor

Alright, merged. Thanks for your patience.

Contributor

jacknagel commented May 5, 2012

Alright, merged. Thanks for your patience.

@jacknagel jacknagel closed this in 1c74a94 May 5, 2012

@MikeMcQuaid

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid May 6, 2012

Member

Thanks @polrop!

Member

MikeMcQuaid commented May 6, 2012

Thanks @polrop!

rohansingh added a commit to rohansingh/homebrew that referenced this pull request May 7, 2012

Honor build options and bottles when upgrading
Fixes #11552.
Closes #11777.

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

This comment has been minimized.

Show comment Hide comment
@nicolasdespres

nicolasdespres May 9, 2012

Contributor

You are welcome!

Contributor

nicolasdespres commented May 9, 2012

You are welcome!

Sharpie pushed a commit to Sharpie/homebrew that referenced this pull request Jun 18, 2012

Honor build options and bottles when upgrading
Fixes #11552.
Closes #11777.

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

snakeyroc3 pushed a commit to snakeyroc3/homebrew that referenced this pull request Dec 17, 2012

Honor build options and bottles when upgrading
Fixes #11552.
Closes #11777.

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

@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 16, 2016

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