Skip to content

Conversation

gcurtis
Copy link
Collaborator

@gcurtis gcurtis commented Sep 13, 2022

Summary

Depends on #106.

When launching a devbox shell, we need to configure the environment so that things don't break in weird ways. We need to copy some variables directly (like LANG/LC_* locale variables) and others we need to modify (such as PATH). Our current method of setting these variables has a few problems:

  1. We don't set things early enough. By the time we set variables in the shellrc, a bunch of commands have already run (and possibly broken).
  2. Setting variables with /usr/bin/env runs into command length limits, especially with the PATH that nix-shell sets.
  3. Non-interactive shells don't run the shellrc at startup, which will be problematic for implementing devbox run or piping scripts to stdin.

To fix this, instead set environment variables when launching the nix-shell process and use the --keep flag to tell Nix to propagate them into the new shell. This ensures that the Nix shell, the Nix shell hooks, and the devbox shell all get the correct environment. See envToKeep for the full list of variables we copy over.

Setting the correct PATH is the most complicated part of this change, but to summarize:

  1. When the user runs devbox shell, we take a snapshot of PATH and NIX_PROFILES.
  2. We split and clean (per filepath.Clean) each path element in these variables.
  3. We remove anything from PATH that is empty, relative, or a subdirectory of anything in NIX_PROFILES.
  4. We stick this new PATH in a variable named PARENT_PATH. In the nix-shell hook, we set PATH="$PATH:$PARENT_PATH.

This ensures that the PATH is correct by the time the devbox shell runs. However, there's still the possibility that the user's shellrc will change the PATH again. This is common with version managers like asdf. To make sure Nix packages always have priority before giving the user a prompt, we run some commands in the devbox shellrc that find all PATH entries that are in the Nix store and put them in front of everything else.

How was it tested?

go test ./... and running a devbox shell.

At the end of the devbox shellrc file, we're setting:

	export PATH="$PURE_NIX_PATH:$ORIGINAL_PATH"

This will overwrite anything that the user's shellrc might've added to
PATH. Change ORIGINAL_PATH to PATH to preserve those changes. There's
also no need for ORIGINAL_PATH to be an environment variable, so set it
directly in the template instead.

To prevent regressions, some tests that generate a devbox shellrc file
and compare it to an expected golden file. This ensures we don't
unknowingly change the way the shellrc is generated.

If we do intentionally change the devbox shellrc, the tests can
automatically update the golden files with -update:

	go test ./nix -update

It's up to the author of the change to verify that any new shellrc files
still work in a real shell.
When launching a devbox shell, we need to configure the environment so
that things don't break in weird ways. We need to copy some variables
directly (like LANG/LC_* locale variables) and others we need to
modify (such as PATH). Our current method of setting these variables
has a few problems:

1. We don't set things early enough. By the time we set variables in the
   shellrc, a bunch of commands have already run (and possibly broken).
2. Setting variables with /usr/bin/env runs into command length limits,
   especially with the PATH that nix-shell sets.
3. Non-interactive shells don't run the shellrc at startup, which will
   be problematic for implementing `devbox run` or piping scripts to
   stdin.

To fix this, instead set environment variables when launching the
`nix-shell` process and use the `--keep` flag to tell Nix to propagate
them into the new shell. This ensures that the Nix shell, the Nix shell
hooks, and the devbox shell all get the correct environment. See
`envToKeep` for the full list of variables we copy over.

Setting the correct PATH is the most complicated part of this change,
but to summarize:

1. When the user runs `devbox shell`, we take a snapshot of PATH and
   NIX_PROFILES.
2. We split and clean (per filepath.Clean) each path element in these
   variables.
3. We remove anything from PATH that is empty, relative, or a
   subdirectory of anything in NIX_PROFILES.
4. We stick this new PATH in a variable named PARENT_PATH. In the
   nix-shell hook, we set `PATH="$PATH:$PARENT_PATH`.

This ensures that the PATH is correct by the time the devbox shell runs.
However, there's still the possibility that the user's shellrc will
change the PATH again. This is common with version managers like asdf.
To make sure Nix packages always have priority before giving the user a
prompt, we run some commands in the devbox shellrc that find all PATH
entries that are in the Nix store and put them in front of everything
else.
@gcurtis gcurtis requested a review from loreto September 13, 2022 21:21
Base automatically changed from gcurtis/shellrc-tests to main September 14, 2022 15:35
@Lagoja Lagoja linked an issue Sep 14, 2022 that may be closed by this pull request
Copy link
Contributor

@loreto loreto left a comment

Choose a reason for hiding this comment

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

LGTM

@gcurtis gcurtis merged commit a5dbdd8 into main Sep 19, 2022
@gcurtis gcurtis deleted the gcurtis/shellenv branch September 19, 2022 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

devbox shell changes locale LC_CTYPE to C
2 participants