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.

Sign up for GitHub

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

Jump to bottom

Fix SystemCommand :path. #4453

Merged
merged 7 commits into from
Jul 11, 2018
Merged

Fix SystemCommand :path. #4453

merged 7 commits into from
Jul 11, 2018

Conversation

reitermarkus
Copy link
Member

@reitermarkus reitermarkus commented Jul 11, 2018 •
edited
Loading

  • 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 tests with your changes locally?

Use :env instead of :path. (Was missed in #4265.)

@reitermarkus reitermarkus requested a review from claui July 11, 2018 07:03
@ghost ghost assigned reitermarkus Jul 11, 2018
@ghost ghost added the in progress Maintainers are working on this label Jul 11, 2018
@reitermarkus reitermarkus force-pushed the fix-system-command branch 4 times, most recently from 9b21b18 to 11cd34e Compare July 11, 2018 07:24
claui
claui suggested changes Jul 11, 2018
View reviewed changes
Copy link
Contributor

@claui claui left a comment

Choose a reason for hiding this comment

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

Looks good to me, except for minor nitpicks.

I’ve also tested installing/uninstalling the relevant cask, and found no regression.

Library/Homebrew/cask/lib/hbc/system_command.rb
end

prefix << "--"
["env", *variables]
Copy link
Contributor

Choose a reason for hiding this comment

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

env → /usr/bin/env

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? The environment is filtered by default, so this shouldn't matter.

Library/Homebrew/test/cask/system_command_spec.rb Outdated
end

subject.run!
context "and sudo: true" do
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this part of the code used to be a bit clearer and easier to read correctly when the contexts were not nested.

A condensed example:

Something something
  and its result
Something something with sudo
  and its result

is easier to read and understand correctly than:

Something something
  and its result
  with sudo
    and its result

Also, un-nesting won't cost us anything here since the code is duplicated anyway.

Library/Homebrew/test/cask/system_command_spec.rb Outdated
it "raises an ArgumentError" do
expect { described_class.run("true", env: { "1ABC" => true }) }
.to raise_error(ArgumentError, /variable name/)
end
end

it "looks for executables in custom PATH" do
Copy link
Contributor

Choose a reason for hiding this comment

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

in custom PATH → in a custom PATH

Library/Homebrew/test/cask/system_command_spec.rb
#!/bin/sh
echo Hello, world!
SH

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we stick with the generic <<~EOS for now, and aim to replace it one day in a single swoop if we have decided on alternatives?

It feels a little confusing to get 98 % EOS (including lots of code snippets) and 2 % heredocs with other identifiers when I grep for <<~[A-Z]+$.

Copy link
Member Author

Choose a reason for hiding this comment

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

With SH I get nested syntax-highlighting in TextMate.

Library/Homebrew/cask/lib/hbc/artifact/installer.rb Outdated
@@ -23,7 +23,7 @@ module ScriptInstaller
def install_phase(command: nil, **_)
ohai "Running #{self.class.dsl_key} script '#{path.relative_path_from(cask.staged_path)}'"
FileUtils.chmod "+x", path unless path.executable?
command.run(path, **args, path: PATH.new(HOMEBREW_PREFIX/"bin", HOMEBREW_PREFIX/"sbin", ENV["PATH"]))
command.run!(path, **args, env: { "PATH" => PATH.new(HOMEBREW_PREFIX/"bin", HOMEBREW_PREFIX/"sbin", ENV["PATH"]) })
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

I wonder whether we could have caught this earlier with an artifact/installer_spec.rb.
I feel this change warrants creating that spec, and have the class under test start out with PATH=/var/dummy, then assert that the system command finds both HOMEBREW_PREFIX/"bin" and /var/dummy in its environment PATH.

(No full coverage needed for now – just the tiny bit that’s being changed here!)

@reitermarkus reitermarkus force-pushed the fix-system-command branch 2 times, most recently from bb24e8f to 845da8e Compare July 11, 2018 14:51
@reitermarkus reitermarkus mentioned this pull request Jul 11, 2018
Closed
6 tasks
@reitermarkus reitermarkus merged commit e1957a9 into Homebrew:master Jul 11, 2018
@reitermarkus reitermarkus deleted the fix-system-command branch July 11, 2018 16:26
@ghost ghost removed the in progress Maintainers are working on this label Jul 11, 2018
@claui
Copy link
Contributor

claui commented Jul 11, 2018

Thanks again @reitermarkus!

@commitay commitay added the cask Homebrew Cask label Jul 11, 2018
@lock lock bot added the outdated PR was locked due to age label Aug 11, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@claui claui claui requested changes

@commitay commitay Awaiting requested review from commitay

Assignees

@reitermarkus reitermarkus

Labels
cask Homebrew Cask outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reitermarkus @claui @commitay