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

Fixes querying Cask commands' help #3306

Merged
merged 13 commits into from Oct 24, 2017

Conversation

Projects
None yet
3 participants
@amyspark
Member

amyspark commented Oct 12, 2017

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

In Homebrew/homebrew-cask#28977, two bugs were discovered (quote from my comment).

The first one:

That last option is being “stolen” by Homebrew itself. Try something like brew cask example --help and you’ll get their help, not even ours.

It's a bug in Cask's cli.rb:

def run(*_args)
purpose
usage
return if @command == "help" && @args.empty?
unknown_command = @args.empty? ? @command : @args.first
raise ArgumentError, "Unknown command: #{unknown_command}"
end

Line 234 specifies that it should print the first argument if available, which the example script does (instead of example, which doesn't exist as a command!)

The second one:

And this looks like a bug, but I just tried brew services --help and my terminal is filled with the (correct) instructions. I then cannot leave/do anything until I ctrlC.

This one only happens if homebrew/services has not been previously tapped, because in this case the help request is redirected to the tap command, which always returns the help text without actually tapping the cask 🤦‍♀️ .
The execution flow is roughly like this:

brew services --help

The bug lies in the remarked step: if a tap is required, it should not obey the HOMEBREW_HELP environment variable.

I've fixed the first bug by specifying all args to be printed along with the offending command, and the second by forcing brew.rb to ignore the shell's HOMEBREW_HELP variable and verify that the help text was actually requested.

Fixes Homebrew/homebrew-cask#28977

amyspark added some commits Oct 12, 2017

Fixes help querying
 - If a non-existent command with a flag is queried to Cask, the latter
is printed instead of the command
 - If the help from a not-yet-tapped cask's command is queried, it
prints brew tap's help infinitely

Fixes Homebrew/homebrew-cask#28977
Show outdated Hide outdated Library/Homebrew/brew.rb Outdated
Show outdated Hide outdated Library/Homebrew/brew.rb Outdated
Fix mistaken &
I meant to verify that both a cmd and a help flag were received from the
shell
Show outdated Hide outdated Library/Homebrew/brew.rb Outdated
Show outdated Hide outdated Library/Homebrew/brew.rb Outdated
Show outdated Hide outdated Library/Homebrew/brew.rb Outdated
Correct setting and deleting HOMEBREW_HELP
(and also delete the extra help_flag case, it's no longer needed)
Show outdated Hide outdated Library/Homebrew/brew.rb Outdated
Show outdated Hide outdated Library/Homebrew/brew.rb Outdated
@amyspark

This comment has been minimized.

Show comment
Hide comment
@amyspark

amyspark Oct 18, 2017

Member

I'm testing the change I made to cli.rb, please do not merge yet.

Member

amyspark commented Oct 18, 2017

I'm testing the change I made to cli.rb, please do not merge yet.

@MikeMcQuaid

This looks good to me now. I'm good to merge when you're 👍, @amyspark.

@amyspark

This comment has been minimized.

Show comment
Hide comment
@amyspark

amyspark Oct 20, 2017

Member

@MikeMcQuaid, I've reverted the lines in cli.rb, I think a better option is to print the first offending command (example) instead of printing the whole Cask command (help example -a).
Please review, and if it is OK, do merge this.
Thanks for your comments!

Member

amyspark commented Oct 20, 2017

@MikeMcQuaid, I've reverted the lines in cli.rb, I think a better option is to print the first offending command (example) instead of printing the whole Cask command (help example -a).
Please review, and if it is OK, do merge this.
Thanks for your comments!

@reitermarkus

This comment has been minimized.

Show comment
Hide comment
@reitermarkus

reitermarkus Oct 20, 2017

Member

I don't think this completely fixes Homebrew/homebrew-cask#28977. Does brew cask -h now show the same as brew cask help?

Member

reitermarkus commented Oct 20, 2017

I don't think this completely fixes Homebrew/homebrew-cask#28977. Does brew cask -h now show the same as brew cask help?

@amyspark

This comment has been minimized.

Show comment
Hide comment
@amyspark

amyspark Oct 20, 2017

Member

@reitermarkus, no, it currently doesn't. The problem is that with the current setup, Homebrew tries to print Cask's help by grepping /usr/local/Homebrew/Library/Homebrew/cmd/cask.rb, which is empty (set HOMEBREW_DEVELOPER to see the warning message). I can't yet find a way to send the request to Cask itself without breaking the brew help command.

Member

amyspark commented Oct 20, 2017

@reitermarkus, no, it currently doesn't. The problem is that with the current setup, Homebrew tries to print Cask's help by grepping /usr/local/Homebrew/Library/Homebrew/cmd/cask.rb, which is empty (set HOMEBREW_DEVELOPER to see the warning message). I can't yet find a way to send the request to Cask itself without breaking the brew help command.

@amyspark

This comment has been minimized.

Show comment
Hide comment
@amyspark

amyspark Oct 21, 2017

Member

Um, I'm not sure why Travis failed with the last commit, it doesn't show anything in the build log.

Member

amyspark commented Oct 21, 2017

Um, I'm not sure why Travis failed with the last commit, it doesn't show anything in the build log.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 21, 2017

Member

Tests failed again after rerun but I'm unable to figure out why.

Member

MikeMcQuaid commented Oct 21, 2017

Tests failed again after rerun but I'm unable to figure out why.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 21, 2017

Member

@reitermarkus What do you think about making Cask subcommand help use the same methods as Homebrew command help rather than just making this a blind passthrough?

Member

MikeMcQuaid commented Oct 21, 2017

@reitermarkus What do you think about making Cask subcommand help use the same methods as Homebrew command help rather than just making this a blind passthrough?

@amyspark

This comment has been minimized.

Show comment
Hide comment
@amyspark

amyspark Oct 21, 2017

Member

@MikeMcQuaid I ran brew tests manually and the test that is failing is cask/cli:46:

it "prints help output when subcommand receives `--help` flag" do
command = described_class.new("noop", "--help")
expect(described_class).to receive(:run_command).with("help", "noop")
command.run
expect(command.help?).to eq(true)
end

EDIT: The reason was that if one issued brew cask -h (as the test does), my commit didn't properly detect that there was no command, thus adding a nil argument and later triggering an exception. Due to the way Cask handles exceptions, the test itself was terminated with 1, thus the reason why there was no error logged.

Member

amyspark commented Oct 21, 2017

@MikeMcQuaid I ran brew tests manually and the test that is failing is cask/cli:46:

it "prints help output when subcommand receives `--help` flag" do
command = described_class.new("noop", "--help")
expect(described_class).to receive(:run_command).with("help", "noop")
command.run
expect(command.help?).to eq(true)
end

EDIT: The reason was that if one issued brew cask -h (as the test does), my commit didn't properly detect that there was no command, thus adding a nil argument and later triggering an exception. Due to the way Cask handles exceptions, the test itself was terminated with 1, thus the reason why there was no error logged.

Show outdated Hide outdated Library/Homebrew/brew.rb Outdated
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 24, 2017

Member

Thanks @amyspark! I'm good with this when @reitermarkus is; can you merge when you're happy (and CI is 💚), thanks!

Member

MikeMcQuaid commented Oct 24, 2017

Thanks @amyspark! I'm good with this when @reitermarkus is; can you merge when you're happy (and CI is 💚), thanks!

@reitermarkus reitermarkus merged commit 632fdca into Homebrew:master Oct 24, 2017

3 checks passed

codecov/patch 100% of diff hit (target 66.9%)
Details
codecov/project 67.9% (+0.99%) compared to 56458f0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reitermarkus

This comment has been minimized.

Show comment
Hide comment
@reitermarkus

reitermarkus Oct 24, 2017

Member

Thank you for your first contribution, @amyspark!

Member

reitermarkus commented Oct 24, 2017

Thank you for your first contribution, @amyspark!

@amyspark amyspark deleted the amyspark:hacktoberfest-quash-cask-help branch Oct 25, 2017

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

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