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

Fix update commit for non-HEAD kegs with head spec #644

Merged
merged 1 commit into from Aug 6, 2016

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?

If we try to call Formulary.from_keg(f, :head) on the keg that
is not HEAD-keg itself, we don't need to update commit of
returned formula and should use just HEAD version with nil commit.

Same is true for ARGV.resolved_formulae

See #500 (comment) and #500 (comment)

CC @UniqMartin @xu-cheng @MikeMcQuaid @eirinikos

If we try to call `Formulary.from_keg(f, :head)` on the keg that
is not HEAD-keg itself, we don't need to update commit of
returned formula and should use just HEAD version with nil commit.

Same is true for `ARGV.resolved_formulae`
@UniqMartin UniqMartin added the gsoc-outreachy Google Summer of Code or Outreachy label Aug 5, 2016
@UniqMartin
Copy link
Contributor

Thanks for the quick fix! However, I think we've just discovered a more fundamental issue, namely that some code in Homebrew allows the spec to be overridden by a command-line argument like --HEAD even if the formula is initialized from an installed keg and the corresponding tab file clearly indicates a different spec for the installed formula. I think we have basically two options to resolve this, both of which diverge from the current behavior:

  • Use the spec from the tab file (if it exists) and ignore the spec given via command-line arguments.
  • Raise an error if the spec in the tab file (if it exists) and the spec given via command-line arguments don't match.

However, I'm not entirely sure of the full ramifications of this change in behavior, thus any feedback on this (and whether this suggestion even makes sense) is greatly appreciated.

@vladshablinsky
Copy link
Contributor Author

vladshablinsky commented Aug 5, 2016

@UniqMartin while I was fixing it I was thinking about something similar (not about the solutions, but about the issue itself). But then I thought what Formulary was used for. Formulary has different methods to generate Formula instance, so I thought that from_keg is needed to get the formula instance of that keg and spec just sets the spec we want that formula to have. After I realised that it didn't seem that strange to me, because it was better than get formula instance and then set the spec manually. That said, maybe I'm missing something.

@xu-cheng
Copy link
Member

xu-cheng commented Aug 6, 2016

@UniqMartin actually I think it's prefectly fine to allow overwrite spec in command line. For example, one would anticipate brew reinstall foo --devel will just reinstall formulae at new spec with all options preserved.

@UniqMartin
Copy link
Contributor

I see. I didn't consider this use case (or other possibly similar use cases). The constellation that revealed the bug addressed in this PR was:

brew install foo
brew test foo --devel
brew test foo --HEAD

Here, only the stable spec is installed, so it feels weird that the spec can be overridden for brew test and, e.g., brew test --devel ends up succeeding despite the spec mismatch. Thoughts?

I guess no matter what the “correct” handling of the above scenario is, this PR is probably a sufficient fix for the observed bug and dealing with spec mismatch is a bit out of scope.

@UniqMartin UniqMartin mentioned this pull request Aug 6, 2016
5 tasks
@xu-cheng xu-cheng merged commit 63c563f into Homebrew:master Aug 6, 2016
@UniqMartin
Copy link
Contributor

Thanks again @vladshablinsky for the quick fix! 🙇

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
gsoc-outreachy Google Summer of Code or Outreachy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants