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

cmd/--prefix: add --installed flag #10266

Merged
merged 1 commit into from Feb 1, 2021

Conversation

gromgit
Copy link
Member

@gromgit gromgit commented Jan 8, 2021

  • 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?
  • Have you successfully run brew man locally and committed any changes?

This changes the output for uninstalled formulae from Cellar prefix to empty string, so:

$ brew --prefix abcde python@3.9 tcl-tk
/usr/local/Cellar/abcde/2.9.3_1
/usr/local/opt/python@3.9
/usr/local/Cellar/tcl-tk/8.6.10
$ brew --prefix --installed abcde python@3.9 tcl-tk

/usr/local/opt/python@3.9

$

Fixes #10261. I chose to print an empty string instead of not printing anything at all, because brew --prefix supports multiple formulae input, so this lets the user tell both visually and programmatically which formulae aren't installed.

For the same reason, "not installed" still returns success as before, so that "no such formula" remains the only error condition After a second think, since the switch is called --installed, it should return failure (to indicate that not everything requested is in place) but not an error message, since it's not an exceptional condition like "no such formula":

$ brew --prefix --installed abcde python@3.9 tlc-tk
Error: No available formula with the name "tlc-tk".
$ brew --prefix --installed abcde python@3.9 tcl-tk || echo "--> Returned failure."

/usr/local/opt/python@3.9

--> Returned failure.

@BrewTestBot
Copy link
Member

Review period will end on 2021-01-11 at 11:45:07 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 8, 2021
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Nice work so far @gromgit!

Library/Homebrew/cmd/--prefix.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/--prefix.rb Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

$ brew --prefix --installed abcde python@3.9 tlc-tk
Error: No available formula with the name "tlc-tk".

Could even consider raising a NotAKegError here if it's not installed.

@gromgit
Copy link
Member Author

gromgit commented Jan 8, 2021

$ brew --prefix --installed abcde python@3.9 tlc-tk
Error: No available formula with the name "tlc-tk".

Could even consider raising a NotAKegError here if it's not installed.

I'm not sure about that. "Formula not installed" is not an exceptional condition, and it's certainly not an unrecoverable one, unlike "no such formula". In fact, I can see a build script doing something like the following:

#!/usr/bin/env bash
shopt -s lastpipe
set -o pipefail
deps=(abcde python@3.9 tcl-tk)
prefixes=()

# Let's see what's already installed
brew --prefix --installed "${deps[@]}" | readarray -t prefixes
for n in "${!deps[@]}"; do
  if [[ -z "${prefixes[$n]}" ]]; then
    # Install what's missing
    brew install "${deps[$n]}"
  fi
done

# Get all the prefixes again, so we can use them for real
brew --prefix --installed "${deps[@]}" | readarray -t prefixes

[...]

@MikeMcQuaid
Copy link
Member

I'm not sure about that. "Formula not installed" is not an exceptional condition, and it's certainly not an unrecoverable one, unlike "no such formula". In fact, I can see a build script doing something like the following:

It feels weird to return an non-zero status code and not say why, is my only concern. We get to define what this outputs/does as we're creating a new flag (and hiding it is only a 2>/dev/null away).

@gromgit
Copy link
Member Author

gromgit commented Jan 8, 2021

It feels weird to return an non-zero status code and not say why, is my only concern.

Fair enough. I've altered the output to list the missing formulae on stderr:

$ brew --prefix --installed abcde python@3.9 tcl-tk 

/usr/local/opt/python@3.9

Warning: The following formulae are not installed:
abcde tcl-tk

$ brew --prefix --installed abcde python@3.9 tcl-tk 2>/dev/null || echo Missing stuff

/usr/local/opt/python@3.9

Missing stuff

@MikeMcQuaid
Copy link
Member

Warning: The following formulae are not installed:
abcde tcl-tk

I feel like at that point NotAKegError is going to provide more consistent behaviour and output. Thoughts?

@gromgit
Copy link
Member Author

gromgit commented Jan 8, 2021

I feel like at that point NotAKegError is going to provide more consistent behaviour and output. Thoughts?

You mean like:

      if args.installed? 
        missing_formulae = formulae.reject { |f| f.opt_prefix.exist? }
                                   .map(&:name)
        return if missing_formulae.empty?

        raise NotAKegError, <<~EOS
          The following formulae are not installed:
          #{missing_formulae.join(" ")}
        EOS
      end

which does this:

$ brew --prefix --installed abcde python@3.9 tcl-tk 

/usr/local/opt/python@3.9

Error: The following formulae are not installed:
abcde tcl-tk

? I don't have a strong opinion either way, but I'd rather respect existing semantics where possible, if only to make it easier for the next person to comprehend the code.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 11, 2021
@BrewTestBot
Copy link
Member

Review period ended.

Library/Homebrew/cmd/--prefix.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/--prefix.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/--prefix.rb Outdated Show resolved Hide resolved
This changes the output for uninstalled formulae from Cellar prefix to empty string, so:
```sh
$ brew --prefix abcde python@3.9 tcl-tk
/usr/local/Cellar/abcde/2.9.3_1
/usr/local/opt/python@3.9
/usr/local/Cellar/tcl-tk/8.6.10
$ brew --prefix --installed abcde python@3.9 tcl-tk

/usr/local/opt/python@3.9

$
```
@MikeMcQuaid
Copy link
Member

Thanks again @gromgit, great work here!

@MikeMcQuaid MikeMcQuaid merged commit cdf8ff9 into Homebrew:master Feb 1, 2021
@gromgit gromgit deleted the prefix_non_installed branch February 1, 2021 12:22
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 4, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fresh brew install returns incorrect value for --prefix openssl
3 participants