Implement #812 (brew configure-shell)

[jamescostian]

• [x] Have you followed the guidelines in our Contributing document?
• [x] Have you checked to ensure there aren't other open Pull Requests for the same change?
• [x] Have you added an explanation of what your changes do and why you'd like us to include them?
• [x] Have you written new tests for your changes? Here's an example.
• [x] Have you successfully run brew style with your changes locally?
• [x] Have you successfully run brew tests with your changes locally?

[sjackman]

The scope of this PR has grown a bit beyond the original proposal. I'd prefer not to ask or set HOMEBREW_DEVELOPER and HOMEBREW_VERBOSE in this PR.

[sjackman]

if MANPATH was empty before running this code, then MANPATH will become $HOMEBREW_PREFIX/share/man: - is that trailing comma ok?

I believe when MANPATH ends in a colon, man also searches the default host directories, which is desirable.

[sjackman]

Looking good, James! Please loop over ~/.profile, ~/.bash_profile, and ~/.zprofile and modify each file if it exists. Do not create the file if it does not exist.

[maxim-belkin]

how do we want to handle C-shell?

[sjackman]

brew configure-shell --help
Warning: No help text in: /home/sjackman/.linuxbrew/Homebrew/Library/Homebrew/cmd/configure-shell.rb

Please add help text. You can look at other commands in cmd/*.rb to see the format.

[sjackman]

Please squash all five commits of this PR down to a single commit. You can do that using

git rebase -i origin/master
git push --force https://github.com/jamescostian/brew

When making further changes to this PR, you can use git commit --amend followed by git push --force to modify the existing commit, rather than creating new ones.

[sjackman]

I tested out your current PR, and it works well! This feature will be a nice improvement to the user installation experience. Thanks!

[jamescostian]

Alright everything should be taken care of except:

• Currently there is no $HOMEBREW_PREFIX/etc/environment.csh or $HOMEBREW_PREFIX/etc/environment.fish
• If you run this command, then you comment out source ... and then run this command again, your ~/.profile will still contain your commented out line (but it will also have another, uncommented source ... line)
• If you run this command, move the brew installation, and run this command again, your ~/.profile will still contain the old source ... line (but it will also have the new source ... line with the correct path)

@sjackman would you like me to tackle these in this PR, or another PR, or leave them unfixed?

[jamescostian]

Hmmm it has now failed the build twice now saying:

Writing markdown to /home/linuxbrew/.linuxbrew/Homebrew/docs/Manpage.md
Writing man page to /home/linuxbrew/.linuxbrew/Homebrew/manpages/brew.1
Writing man page to /home/linuxbrew/.linuxbrew/Homebrew/manpages/brew-cask.1
==> FAILED

Does anyone know how to fix that?

[jonchang]

brew man then commit the changes.

[sjackman]

Those three issues that you've raised can remain unaddressed by this PR.

[sjackman]

Looks good to me! Thanks, James!

One thought though: if $HOMEBREW_PREFIX/etc/environment already exists, and its contents are not equal to those that are about to be written, it should instead print an error message:

Error: ~/.linuxbrew/etc/environment already exists. If you wish to replace this file, remove this file and run brew configure-shell again.

If the contents are identical, there's no need to print the error message.

Print the error message using odie "message goes here" or

odie <<~EOS
  message goes here
EOS

[maxim-belkin]

why do you not use export_value and profile that are defined in Library/Homebrew/utils/shell.rb?

[jamescostian]

@jonchang Thanks! I ran brew man and took the changes it suggested relating to this command. It also suggested some changes for other commands - should I be accepting those changes as well?

@maxim-belkin I agree, and I suggested that here but I didn't get a green light in that thread - @sjackman what do you think? As Max mentioned, using export_value would mean less code duplication and would make it easier to have this command also make environment.csh and environment.fish

EDIT: you could also use SHELL_PROFILE_MAP to get a list of files that should source environment (if they exist), which would be better for having less code duplication

[jamescostian]

@sjackman your request that environment should not be overwritten if it exists has been taken care of, and the error message (that only shows if there are different contents) has also been added :tada:

[sjackman]

Looks good!

Please print informational messages about which shell profile files have been modified. For example…

Modified ~/.profile
Skipped ~/.bash_profile because it's already configured

Please add to the end…

oh1 "To configure your current shell session, please run..."
puts "  source #{HOMEBREW_PREFIX}/etc/environment"

This message should be printed whether or not that file is updated.

[maxim-belkin]

linux-pam installs HOMEBREW_PREFIX/etc/environment

[sjackman]

Argh! I was worried that some other package would install that file! Thanks for catching this issue, Maxim. The default contents of the file are empty (except for comments):

#
# This file is parsed by pam_env module
#
# Syntax: simple "KEY=VAL" pairs on separate lines
#

All the same though, I think it'd be safer to use a different file name. I suggest #{HOMEBREW_PREFIX}/etc/brew.env

[jamescostian]

Alright, all of that has been taken care of now 👍

[sjackman]

Looking good! Two final fixes:

Add $HOMEBREW_PREFIX/sbin to the PATH after bin.

If no shell profile files currently exist, create a new file ~/.profile and modify it.

[jamescostian]

Good points! They are now taken care of

[sjackman]

Also note these brew style issues:

/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/test/cmd/configure-shell.rb:19:7: C: Align .and with .to_stdout on line 18.
      .and not_to_output.to_stderr
      ^^^^
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/test/cmd/configure-shell.rb:29:7: C: Align .and with .to_stdout on line 28.
      .and not_to_output.to_stderr
      ^^^^
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/test/cmd/configure-shell.rb:41:7: C: Align .and with .to_stdout on line 40.
      .and not_to_output.to_stderr
      ^^^^
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/test/cmd/configure-shell.rb:45:7: C: Align .and with .to_stdout on line 44.
      .and not_to_output.to_stderr
      ^^^^

https://circleci.com/gh/Linuxbrew/brew/1212

[sjackman]

Once this feature has made it into a stable release of Linuxbrew/brew, I'll update Linuxbrew/install and the README.md of Linuxbrew/brew to use it!

Once this PR is merged and has some testing with users, you can also submit it upstream to Homebrew/brew, if you like. (I'd appreciate it.) You can mention that it's primarily useful for Linux users, but is also useful for non-/usr/local installs on macOS. I don't have write permissions to /usr/local on my machine at work (due to be a clinical laboratory), so I have Homebrew installed in ~/.homebrew, so this tool is useful on macOS as well in that situation.

[jamescostian]

@sjackman Thanks! And yeah, after this PR merges I'll make the same one to Homebrew/brew

The brew style issues are weird... I don't see them on my machine (I'm doing this development in an Ubuntu docker container), and fixing them requires the code to look pretty ugly, e.g.:

    expect { run_cmd }
      .to be_a_success
      .and output(%r{Modified ~\/\.profile}).to_stdout
                                            .and not_to_output.to_stderr

Any ideas why this is happening?

[sjackman]

No, I don't know why that's happening. That same style is used in other tests, where it seems fine. To make brew style happy, I suggest…

    expect { run_cmd }
      .to be_a_success
      .and output(%r{Modified ~\/\.profile})
      .to_stdout
      .and not_to_output.to_stderr

[sjackman]

 .and output(%r{Modified ~\/\.profile})

The forward slash / does not need to be escaped (I believe).

[jamescostian]

Alright those two things should be taken care of now

[sjackman]

The last run of CircleCI took 27 minutes to run. That's unusually long. https://circleci.com/gh/Linuxbrew/brew/1218

[sjackman]

Thank you for this most excellent contribution to Linuxbrew, James! 🏆

[maxim-belkin]

The last run of CircleCI took 27 minutes to run

It did a very thorough test, I guess.