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

formula_installer: improve no-bottle error message #10183

Merged
merged 2 commits into from Dec 31, 2020

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Dec 30, 2020

  • 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 style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
    • This returned a java_spec error and a tap-new_spec error. brew tests produces these errors even when run on the master branch.
  • Have you successfully run brew man locally and committed any changes?

Closes #10180.

This PR changes the error message when you try to install python@3.9 and you have no CLT installed to:

❯ HOMEBREW_NO_AUTO_UPDATE=1 brew install python@3.9
Error: python@3.9: unable to pour bottle!
The bottle needs the Apple Command Line Tools to be installed.
  You can install them, if desired, with:
    xcode-select --install
You can try to install from source with e.g.
  brew install --build-from-source python@3.9
Please note building from source is unsupported. You will encounter build
failures with some formulae. If you experience any issues please create pull
requests instead of asking for help on Homebrew's GitHub, Twitter or any other
official channels.

If the formula does not specify a pour_bottle? method but also has no bottles:

❯ HOMEBREW_NO_AUTO_UPDATE=1 brew install a2ps
Error: a2ps: unable to pour bottle!
You can try to install from source with e.g.
  brew install --build-from-source a2ps
Please note building from source is unsupported. You will encounter build
failures with some formulae. If you experience any issues please create pull
requests instead of asking for help on Homebrew's GitHub, Twitter or any other
official channels.

I suspect there may be more refactoring in order for the pour_bottle? method in formula_installer.rb, but I'd prefer to address those later.

@BrewTestBot
Copy link
Member

Review period will end on 2020-12-31 at 14:52:26 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 30, 2020
Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

I think the no bottle available! message is a tad misleading in this case. Maybe instead: bottle conditions not met!. No bottle available feels more "final" as if it's saying "sorry there's nothing you can do except build from source" while this is more like "before you can use a bottle you must do this".

Maybe that's not the case for all formulae that use pour_bottle?, though.

@carlocab
Copy link
Member Author

How about no suitable bottle available?

Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

Sorry, wasn't clear with what I was thinking.

I was thinking this "unable to pour bottle" message would be only for instances where formula.pour_bottle? is false.

When this message is showing in situations like no ARM bottles are available, I think it makes sense to still have the "no bottle available" message. That's a little more accurate to what the exact issue is for the user, I think.

@carlocab
Copy link
Member Author

carlocab commented Dec 31, 2020

I agree, but I'm still mulling over the best way to implement it.

For instance, maybe it's enough to just add && formula.pour_bottle? to the conditions required by the if statement and drop any of the changes I've made here so far.

Alternatively, I could add an else after the second message += to state explicitly that there isn't a bottle...

@Rylan12
Copy link
Member

Rylan12 commented Dec 31, 2020

Ah, you know what? I think this is what you want to do instead:

Change the !pour_bottle? condition on line 244 (just above you changes) to !pour_bottle?(output_warning: true)

You may also need to adjust the messaging. See if that gives a better result. pour_bottle? is already calling formula.pour_bottle? and it displays the message there.

@carlocab
Copy link
Member Author

I've considered that too, but decided against it since adjusting the messaging already in the pour_bottle? method seemed overly complicated... but I'll think about it again.

@carlocab
Copy link
Member Author

Thinking about it some more, I don't think I should change the messaging in the pour_bottle? method, since that has implications on Linux as well. Whatever changes need to be made will have to be where I've already proposed them in this PR. Still thinking about the right way to do it though.

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Dec 31, 2020
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 31, 2020
@MikeMcQuaid MikeMcQuaid merged commit 881cd6a into Homebrew:master Dec 31, 2020
@carlocab carlocab deleted the clt-bottles branch December 31, 2020 15:40
carlocab added a commit to carlocab/brew that referenced this pull request Jan 4, 2021
Follow up to Homebrew#10183.

This improves the error message displayed when `formula.pour_bottle?` is
false.

Before:

    ❯ brew install python@3.8
    Error: python@3.8: no bottle available!
    The bottle needs the Apple Command Line Tools to be installed.
      You can install them, if desired, with:
        xcode-select --install
    You can try to install from source with e.g.
      brew install --build-from-source python@3.8
    Please note building from source is unsupported. You will encounter build
    failures with some formulae. If you experience any issues please create pull
    requests instead of asking for help on Homebrew's GitHub, Twitter or any other
    official channels.

After:

    ❯ brew install python@3.8
    Error: python@3.8: the bottle needs the Apple Command Line Tools to be installed.
      You can install them, if desired, with:
        xcode-select --install
    You can try to install from source with e.g.
      brew install --build-from-source python@3.8
    Please note building from source is unsupported. You will encounter build
    failures with some formulae. If you experience any issues please create pull
    requests instead of asking for help on Homebrew's GitHub, Twitter or any other
    official channels.
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 31, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No clear message given when CLT is needed for a bottle
4 participants