Skip to content
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

Test do fix for HEAD versions. #536

Merged
merged 1 commit into from Jul 17, 2016
Merged

Conversation

vladshablinsky
Copy link
Contributor

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

Fixes #535

@vladshablinsky vladshablinsky changed the title Test do fix Test do fix for HEAD versions. Jul 17, 2016
@vladshablinsky
Copy link
Contributor Author

cc @xu-cheng @ilovezfs .

@@ -166,6 +166,10 @@ def initialize(name, path, spec)
validate_attributes!
@build = active_spec.build
@pin = FormulaPin.new(self)

if head && head_pkg_version = latest_head_pkg_version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean ==?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this checks whether latest_head_pkg_version is nil or not i.e. there is at least one HEAD installed. I use head_pkg_version in the following line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, in that case you should probably do the assignment on its own line.

@xu-cheng
Copy link
Member

xu-cheng commented Jul 17, 2016

As I explained in the above comment, this is an incorrect fix.

What you really need to do is to update commit inside ARGV.resolved_formulae and Formulary.from_keg, that's the place the resolution logic lies in. Therefore, here is my thought of solution:

diff --git a/Library/Homebrew/extend/ARGV.rb b/Library/Homebrew/extend/ARGV.rb
index beee470..2da6141 100644
--- a/Library/Homebrew/extend/ARGV.rb
+++ b/Library/Homebrew/extend/ARGV.rb
@@ -32,6 +32,10 @@ module HomebrewArgvExtension
           resolved_spec = spec(nil) || tab.spec
           f.set_active_spec(resolved_spec) if f.send(resolved_spec)
           f.build = tab
+          if f.head? && tab.tabfile
+            k = Keg.new(tab.tabfile.parent)
+            f.version.update_commit(k.version.version.commit)
+          end
         end
         f
       else
diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb
index 4254437..20f115f 100644
--- a/Library/Homebrew/formulary.rb
+++ b/Library/Homebrew/formulary.rb
@@ -246,6 +246,7 @@ class Formulary
       end
     end
     f.build = tab
+    f.version.update_commit(keg.version.version.commit) if f.head?
     f
   end

diff --git a/Library/Homebrew/postinstall.rb b/Library/Homebrew/postinstall.rb
index 14e5781..0b6d8f6 100644
--- a/Library/Homebrew/postinstall.rb
+++ b/Library/Homebrew/postinstall.rb
@@ -11,7 +11,7 @@ begin

   trap("INT", old_trap)

-  formula = ARGV.formulae.first
+  formula = ARGV.resolved_formulae.first
   formula.extend(Debrew::Formula) if ARGV.debug?
   formula.run_post_install
 rescue Exception => e
diff --git a/Library/Homebrew/test.rb b/Library/Homebrew/test.rb
index 796ce09..ffffa18 100644
--- a/Library/Homebrew/test.rb
+++ b/Library/Homebrew/test.rb
@@ -19,7 +19,7 @@ begin

   trap("INT", old_trap)

-  formula = ARGV.formulae.first
+  formula = ARGV.resolved_formulae.first
   formula.extend(Homebrew::Assertions)
   formula.extend(Debrew::Formula) if ARGV.debug?

@vladshablinsky
Copy link
Contributor Author

Agreed.

@xu-cheng xu-cheng merged commit 092d471 into Homebrew:master Jul 17, 2016
souvik1997 pushed a commit to souvik1997/brew that referenced this pull request Jul 25, 2016
@mariusk
Copy link

mariusk commented Aug 6, 2016

When I try to build emacs with --HEAD, the build fails with:

Error: undefined method `commit' for #<Version:0x007fb931c24a10 @version="25.1-rc1">
Please report this bug:
    https://git.io/brew-troubleshooting
/usr/local/Library/Homebrew/formulary.rb:249:in `from_keg'
/usr/local/Library/Homebrew/formulary.rb:225:in `from_rack'
/usr/local/Library/Homebrew/extend/ARGV.rb:43:in `block in resolved_formulae'
/usr/local/Library/Homebrew/extend/ARGV.rb:27:in `map'
/usr/local/Library/Homebrew/extend/ARGV.rb:27:in `resolved_formulae'
/usr/local/Library/Homebrew/cmd/reinstall.rb:11:in `reinstall'
/usr/local/Library/Homebrew/brew.rb:87:in `<main>'

I do not know much about homebrew internals, but by looking at this commit it seems ARGV.rb does a bit more before calling update_commit than formulary.rb, leading me to speculate that maybe some additional checks and updates are needed for formulary.rb as well (if I just comment out the line calling update_commit the build seems build fine)?

@ilovezfs
Copy link
Contributor

ilovezfs commented Aug 6, 2016

@mariusk I think you need to brew update.

@mariusk
Copy link

mariusk commented Aug 6, 2016

I could have sworn I did it already, but now that I tested and rebuilt emacs it seems to work. Thanks, my bad!

@UniqMartin
Copy link
Contributor

I think you need to brew update.

However, if updating doesn't resolve this build failure, I suspect this is another manifestation of the bug that is about to be fixed in PR #644.

@UniqMartin
Copy link
Contributor

Yes, indeed. Now that #644 is merged, this should be fixed after a brew update. But prior to that, this sequence of commands would have triggered the failure:

$ brew install emacs --without-librsvg --devel
[…]
🍵   /opt/brewery/dummy/Cellar/emacs/25.1-rc1: 4,038 files, 97.6M, built in 3 minutes 6 seconds
$ brew reinstall emacs --HEAD
Error: undefined method `commit' for #<Version:0x007fd3e32c5538 @version="25.1-rc1">
Please report this bug:
    https://git.io/brew-troubleshooting
/opt/brewery/dummy/Library/Homebrew/formulary.rb:249:in `from_keg'
/opt/brewery/dummy/Library/Homebrew/formulary.rb:225:in `from_rack'
/opt/brewery/dummy/Library/Homebrew/extend/ARGV.rb:43:in `block in resolved_formulae'
/opt/brewery/dummy/Library/Homebrew/extend/ARGV.rb:27:in `map'
/opt/brewery/dummy/Library/Homebrew/extend/ARGV.rb:27:in `resolved_formulae'
/opt/brewery/dummy/Library/Homebrew/cmd/reinstall.rb:11:in `reinstall'
/opt/brewery/dummy/Library/Homebrew/brew.rb:87:in `<main>'

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants