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

Idempotent setup instructions #559

Closed
carlocab opened this issue Jul 24, 2021 · 5 comments · Fixed by Homebrew/brew#11789
Closed

Idempotent setup instructions #559

carlocab opened this issue Jul 24, 2021 · 5 comments · Fixed by Homebrew/brew#11789
Labels

Comments

@carlocab
Copy link
Member

On Linux and ARM macOS, we ask users to run some variation of these commands:

echo 'eval $(/opt/homebrew/bin/brew shellenv)' >> ~/.zprofile
eval $(/opt/homebrew/bin/brew shellenv)

These commands provide no feedback, so it becomes tempting for users to run them more than once (cf. Homebrew/discussions#1871). This is not ideal because the first command is not idempotent.

I suggest we find a way to make adding brew to a user's PATH 1) idempotent, and 2) provide feedback that this has been completed successfully.

Achieving both of these things might require adding a new command to brew, but it would be nicer if we could get this done with just the install script.

If we don't want to add a new command, we may want to consider tweaking our post-installation instructions to be a bit more handhold-y for users who are less comfortable with working in the terminal. (We'll probably have to give up idempotence, but we can maybe live without it.)

@gromgit
Copy link
Member

gromgit commented Jul 24, 2021

Getting line 1 done in the install script shouldn't be too difficult (line 2 obviously needs to be done manually no matter what), and I'd be happy to take a crack at it, but perhaps we should first revisit the reason why this was not automated in the first place.

And I have no idea what that reason is. 🤷‍♂️

@carlocab
Copy link
Member Author

I really don't want to automate the first line:

The different ways you can configure your startup scripts are manifold: maybe you use .{z,bash_}profile, .{z,ba}shrc, or maybe even .zshenv (but hopefully not). The structure of these scripts can vary too, and it's too hard to modify them in a way that respects that structure.

I also just remembered the following: we don't write outside HOMEBREW_PREFIX, and I think it's good to keep things that way. Writing to a user's shell profile has security implications I'd rather not explore. This is, I think, a strike against my idea to add an extra command that does this.

@gromgit
Copy link
Member

gromgit commented Jul 24, 2021

I understand your concerns, and here's my POV:

  1. We're already telling the user where to write that line, so if that happens to be the wrong place, that needs to be fixed regardless.
  2. Asking the user to manually run the multiple commands needed to check-then-apply-then-check-again is probably even harder to fool-proof than the current instructions.
  3. The install script is already writing outside HOMEBREW_PREFIX in a manner of speaking: HOMEBREW_CACHE is automatically created under the install user's home directory.

So here's what I'm proposing the install script should do:

  1. Search each shell's set of interactive login startup files for brew shellenv.
  2. If not found anywhere, auto-append line 1 to the first one that exists (default: ~/.profile), check that it was written correctly, and report to user what was done.
  3. If found, report to user where it was found, and noting that no change was made.

For uninitiated CLI users, that should maximize the chance of It Just Works. For the experienced, they're given enough information to tweak after the fact if needed.

@XuehaiPan
Copy link
Contributor

XuehaiPan commented Jul 24, 2021

I think we can learn from the setup method of conda:

$ conda init -h 
usage: conda init [-h] [--all] [--reverse] [--json] [-v] [-q] [-d] [shells [shells ...]]

Initialize conda for shell interaction. [Experimental]

Options:

positional arguments:
  shells         One or more shells to be initialized. If not given, the default value is 'bash' on unix and 'cmd.exe' on
                 Windows. Use the '--all' flag to initialize all shells. Currently compatible shells are {bash, fish,
                 powershell, tcsh, xonsh, zsh}

optional arguments:
  -h, --help     Show this help message and exit.
  --all          Initialize all currently available shells.
  -d, --dry-run  Only display what would have been done.

setup type:
  --reverse      Undo past effects of conda init.

Output, Prompt, and Flow Control Options:
  --json         Report all output as json. Suitable for using conda programmatically.
  -v, --verbose  Use once for info, twice for debug, three times for trace.
  -q, --quiet    Do not display progress bar.

Key parts of conda's functionality require that it interact directly with the shell
within which conda is being invoked. The `conda activate` and `conda deactivate` commands
specifically are shell-level commands. That is, they affect the state (e.g. environment
variables) of the shell context being interacted with. Other core commands, like
`conda create` and `conda install`, also necessarily interact with the shell environment.
They're therefore implemented in ways specific to each shell. Each shell must be configured
to make use of them.

This command makes changes to your system that are specific and customized for each shell.
To see the specific files and locations on your system that will be affected before, use the
'--dry-run' flag.  To see the exact changes that are being or will be made to each location,
use the '--verbose' flag.

IMPORTANT: After running `conda init`, most shells will need to be closed and restarted
           for changes to take effect.

$ conda init
no change     /home/PanXuehai/Miniconda3/condabin/conda
no change     /home/PanXuehai/Miniconda3/bin/conda
no change     /home/PanXuehai/Miniconda3/bin/conda-env
no change     /home/PanXuehai/Miniconda3/bin/activate
no change     /home/PanXuehai/Miniconda3/bin/deactivate
no change     /home/PanXuehai/Miniconda3/etc/profile.d/conda.sh
no change     /home/PanXuehai/Miniconda3/etc/fish/conf.d/conda.fish
no change     /home/PanXuehai/Miniconda3/shell/condabin/Conda.psm1
no change     /home/PanXuehai/Miniconda3/shell/condabin/conda-hook.ps1
no change     /home/PanXuehai/Miniconda3/lib/python3.8/site-packages/xontrib/conda.xsh
no change     /home/PanXuehai/Miniconda3/etc/profile.d/conda.csh
modified      /home/PanXuehai/.bashrc

==> For changes to take effect, close and re-open your current shell. <==

$ conda init --all
no change     /home/PanXuehai/Miniconda3/condabin/conda
no change     /home/PanXuehai/Miniconda3/bin/conda
no change     /home/PanXuehai/Miniconda3/bin/conda-env
no change     /home/PanXuehai/Miniconda3/bin/activate
no change     /home/PanXuehai/Miniconda3/bin/deactivate
no change     /home/PanXuehai/Miniconda3/etc/profile.d/conda.sh
no change     /home/PanXuehai/Miniconda3/etc/fish/conf.d/conda.fish
no change     /home/PanXuehai/Miniconda3/shell/condabin/Conda.psm1
no change     /home/PanXuehai/Miniconda3/shell/condabin/conda-hook.ps1
no change     /home/PanXuehai/Miniconda3/lib/python3.8/site-packages/xontrib/conda.xsh
no change     /home/PanXuehai/Miniconda3/etc/profile.d/conda.csh
no change     /home/PanXuehai/.bashrc
no change     /home/PanXuehai/.zshrc
modified      /home/PanXuehai/.config/fish/config.fish
modified      /home/PanXuehai/.xonshrc
modified      /home/PanXuehai/.tcshrc
modified      /home/PanXuehai/.config/powershell/profile.ps1

==> For changes to take effect, close and re-open your current shell. <==

The core function here conda/core/initialize.py.

We can provide a command (e.g. brew init [shell]) in brew for the users. And the installer script just prompts that:

==> Next steps:
Run the following command in the terminal to initialize your shell:
    ~/.linuxbrew/bin/brew init  # or /opt/homebrew/bin/brew init

I think this is more clear for users and the brew init command can check the dotfiles and avoid duplicate initialization.

Further, we can run the initialize instructions for users in the installer. The conda installer:

if [ "$BATCH" = "0" ]; then
    # Interactive mode.
    BASH_RC="$HOME"/.bashrc
    DEFAULT=no
    printf "Do you wish the installer to initialize Miniconda3\\n"
    printf "by running conda init? [yes|no]\\n"
    printf "[%s] >>> " "$DEFAULT"
    read -r ans
    if [ "$ans" = "" ]; then
        ans=$DEFAULT
    fi
    if [ "$ans" != "yes" ] && [ "$ans" != "Yes" ] && [ "$ans" != "YES" ] && \
       [ "$ans" != "y" ]   && [ "$ans" != "Y" ]
    then
        printf "\\n"
        printf "You have chosen to not have conda modify your shell scripts at all.\\n"
        printf "To activate conda's base environment in your current shell session:\\n"
        printf "\\n"
        printf "eval \"\$($PREFIX/bin/conda shell.YOUR_SHELL_NAME hook)\" \\n"
        printf "\\n"
        printf "To install conda's shell functions for easier access, first activate, then:\\n"
        printf "\\n"
        printf "conda init\\n"
        printf "\\n"
    else
        if [[ $SHELL = *zsh ]]
        then
            $PREFIX/bin/conda init zsh
        else
            $PREFIX/bin/conda init
        fi
    fi
    printf "If you'd prefer that conda's base environment not be activated on startup, \\n"
    printf "   set the auto_activate_base parameter to false: \\n"
    printf "\\n"
    printf "conda config --set auto_activate_base false\\n"
    printf "\\n"

    printf "Thank you for installing Miniconda3!\\n"
fi # !BATCH

@MikeMcQuaid
Copy link
Member

I really don't want to automate the first line:

Agreed and other commenters online seem to.

I suggest we find a way to make adding brew to a user's PATH 1) idempotent, and 2) provide feedback that this has been completed successfully.

Simple solution: don't add it to their PATH if their PATH already contains it.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants