Skip to content

Fix brew cask doctor for non-standard installation location#4320

Merged
reitermarkus merged 2 commits intoHomebrew:masterfrom
lucafavatella:brew-cask-doctor
Jun 18, 2018
Merged

Fix brew cask doctor for non-standard installation location#4320
reitermarkus merged 2 commits intoHomebrew:masterfrom
lucafavatella:brew-cask-doctor

Conversation

@lucafavatella
Copy link
Copy Markdown
Contributor

  • Have you followed the guidelines in our Contributing document?
    • I ran brew update several times.
    • brew doctor shows one warning Warning: Your Homebrew's prefix is not /usr/local.
      • (BTW I would need to find out how to make the exit status of brew doctor not to be non-zero only for the non-standard prefix - for automated check.)
    • This is not a formula-specific issue.
  • 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?
    • Please refer to commit message. Please let me know if you want me to move symptom&analysis from commit message to PR description.
  • Have you written new tests for your changes? Here's an example.
    • No. I had a look at the tests in Library/Homebrew/test/cmd/doctor_spec.rb though I am not sure how I would write a test for this fix. Guidance welcome.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?
    • I get one failure - irrespective of my changes:
  1) OS::Mac::language can be parsed by Locale::parse
     Failure/Error: expect { Locale.parse(subject.language) }.not_to raise_error

       expected no Exception, got #<Locale::ParserError: '' cannot be parsed to a Locale> with backtrace:
         # ./locale.rb:15:in `parse'
         # ./test/os/mac_spec.rb:19:in `block (4 levels) in <top (required)>'
         # ./test/os/mac_spec.rb:19:in `block (3 levels) in <top (required)>'
         # ./test/spec_helper.rb:117:in `block (2 levels) in <top (required)>'
         # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec/retry.rb:120:in `block in run'
         # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec/retry.rb:109:in `loop'
         # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec/retry.rb:109:in `run'
         # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
         # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec/retry.rb:34:in `block (2 levels) in setup'
         # ./vendor/bundle/ruby/2.3.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'
     # ./test/os/mac_spec.rb:19:in `block (3 levels) in <top (required)>'
     # ./test/spec_helper.rb:117:in `block (2 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec/retry.rb:120:in `block in run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec/retry.rb:109:in `loop'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec/retry.rb:109:in `run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.0/lib/rspec/retry.rb:34:in `block (2 levels) in setup'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'

I think the test is making a wrong assumption on system configuration, or my system has broken language config:

$ defaults read -g AppleLanguages
2018-06-10 00:37:23.861 defaults[17535:160332]
The domain/default pair of (kCFPreferencesAnyApplication, AppleLanguages) does not exist

@commitay commitay added the cask Homebrew Cask label Jun 9, 2018
@commitay commitay requested a review from reitermarkus June 9, 2018 23:57
@lucafavatella
Copy link
Copy Markdown
Contributor Author

I see codecov/patch job failed but I have no idea whether it is expected, and it is unclear to me how I may address it. These changes delete some unused code so I would expect code coverage percentage to increase rather.

@reitermarkus
Copy link
Copy Markdown
Member

I think the test is making a wrong assumption on system configuration, or my system has broken language config:

The latter, mine shows

$ defaults read -g AppleLanguages
(
    "de-AT",
    de,
    en
)

Try adding/deleting a system language in System Preferences.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should also use user_tilde.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed. Rebased on master and force-pushed.

Symptom (relevant portions):
```
$ brew cask doctor
==> Homebrew-Cask Version
Homebrew-Cask 1.6.7-56-g9ebcef7
Homebrew/homebrew-cask (git revision fc24e; last commit 2018-06-09)
==> macOS
10.13.5
==> SIP
Enabled
...
==> Homebrew-Cask Install Location
<NONE>
==> Homebrew-Cask Staging Location
~/homebrew/Caskroom
...
==> Environment Variables
HOMEBREW_CASK_OPTS="--appdir=~/Applications"
LC_ALL="en_US.UTF-8"
PATH="/usr/bin:/bin:/usr/sbin:/sbin:~/homebrew/Library/Homebrew/shims/scm"
SHELL="/bin/bash"
Cask's Doctor Checkup: failed
 - The staging path ~/homebrew/Caskroom does not exist.
Error: There are some problems with your setup.
```

Analysis:

* The source code is
  [this](https://github.com/Homebrew/brew/blob/9ebcef785eed097be709bffef97263b5f536ba34/Library/Homebrew/cask/lib/hbc/cli/doctor.rb#L63-L66).

* The issue is reproducible in `brew irb`:

```
$ ls -dl ~/homebrew/Caskroom
drwxrwxr-x  10 luca  admin  340  9 Jun 16:22 /Users/luca/homebrew/Caskroom
$ ( cd ~ && pwd; )
/Users/luca
$ ls -dl /Users/luca/homebrew/Caskroom
drwxrwxr-x  10 luca  admin  340  9 Jun 16:22 /Users/luca/homebrew/Caskroom
$ brew irb
==> Interactive Homebrew Shell
Example commands available with: brew irb --examples
irb(main):001:0> Pathname.new("~/homebrew/Caskroom").exist?
=> false
irb(main):002:0> Pathname.new("/Users/luca/homebrew/Caskroom").exist?
=> true
```
@reitermarkus reitermarkus merged commit c814199 into Homebrew:master Jun 18, 2018
@reitermarkus
Copy link
Copy Markdown
Member

reitermarkus commented Jun 18, 2018

Thanks, @lucafavatella, nice cleanup!

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

Labels

cask Homebrew Cask outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants