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

Set IN_NIX_SHELL=1 for tests #7800

Closed
wants to merge 1 commit into from
Closed

Conversation

booxter
Copy link

@booxter booxter commented Aug 29, 2024

This helps isolating system-wide zsh instance running in tests from the broader system shell initialization for limited nix environments.

This fixes failures as reported here:
NixOS/nixpkgs#312692

This helps isolating system-wide zsh instance running in tests from the
broader system shell initialization for limited nix environments.

This fixes failures as reported here:
NixOS/nixpkgs#312692

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
@kovidgoyal
Copy link
Owner

What exactly does IN_NIX_SHELL do and can you please add a comment explaining what it does in basic_shell_env()

@kovidgoyal
Copy link
Owner

As far as I can tell from a quick google valid values for it are pure/impure and its effects are undefined, depending on individual pkgs. https://nix.dev/manual/nix/2.18/command-ref/nix-shell#env-IN_NIX_SHELL

@booxter
Copy link
Author

booxter commented Aug 29, 2024

It very well may be that this is not a correct fix. Let me describe what I'm trying to achieve and why this helps.

The reason why test suite fails is that while zsh in the nix build environment comes from an injected package, it still reads from /etc/zshenv. From the man page for zsh:

STARTUP/SHUTDOWN FILES
       Commands are first read from /etc/zshenv; **this cannot  be
       overridden**.   Subsequent behaviour is modified by the RCS
       and GLOBAL_RCS options; the former  affects  all  startup
       files, while the second only affects global startup files
       (those  shown  here  with an path starting with a /).  If
       one of the options is unset at any point, any  subsequent
       startup  file(s)  of  the  corresponding type will not be
       read.  It is also possible for  a  file  in  $ZDOTDIR  to
       re-enable  GLOBAL_RCS. Both RCS and GLOBAL_RCS are set by
       default.

So tests adding --noglobalrcs is not enough to isolate in-test zsh from the system.

And here is what is in /etc/zshenv (managed by nix-darwin):

$ cat /etc/zshenv
# /etc/zshenv: DO NOT EDIT -- this file has been generated automatically.
# This file is read for all shells.

# Only execute this file once per shell.
# But don't clobber the environment of interactive non-login children!
if [ -n "$__ETC_ZSHENV_SOURCED" ]; then return; fi
export __ETC_ZSHENV_SOURCED=1

# Don't execute this file when running in a pure nix-shell.
if test -n "$IN_NIX_SHELL"; then return; fi

if [ -z "$__NIX_DARWIN_SET_ENVIRONMENT_DONE" ]; then
  . /nix/store/5kgqn44swwbivdxx1gsvpkz1qcvfhq16-set-environment
fi



# Read system-wide modifications.
if test -f /etc/zshenv.local; then
  source /etc/zshenv.local
fi

If the variable is not set, my full zsh environment is initialized, which triggers the following command (comes from /nix/store/5kgqn44swwbivdxx1gsvpkz1qcvfhq16-set-environment):

/nix/store/0mnp04vh94qi0cir0qhy29dn675hmpj5-gnupg-2.4.5/bin/gpg-connect-agent --quiet updatestartuptty /bye > /dev/null

...which produces some errors on the screen and make the tests fail.

Setting IN_NIX_SHELL allows me to work around the issue since then my env is not initialized.

@booxter
Copy link
Author

booxter commented Aug 29, 2024

I'm looking at this a bit closer, and I wonder if perhaps the root of the issue is not in the test suite, or not even in zsh, but in the fact that nix-darwin configures its zshenv such that it may dump errors from gpg-connect-agent in some scenarios.

Here is where the command is injected into the env: https://github.com/LnL7/nix-darwin/blob/ac5694a0b855a981e81b4d9f14052e3ff46ca39e/modules/programs/gnupg.nix#L46

I wonder if it should be gpg-connect-agent --quiet updatestartuptty /bye > /dev/null 2>&1 (note the additional 2>&1) to suppress all errors.

Do you think I should do this instead?

@kovidgoyal
Copy link
Owner

Yes, generally speaking, having things that potentially spew errors in
global rc files is not a good idea. Lots of tools run shell scripts and
can be confused by errors that dont come from the script itself.

Closing as IMO this should be fixed in nix.

@kovidgoyal kovidgoyal closed this Aug 29, 2024
booxter added a commit to booxter/nix-darwin that referenced this pull request Aug 29, 2024
In some scenarios, the command may fail, e.g. when the shell is executed
with a different $HOME from where gpg agent is configured to run from.

(E.g. this happens in kitty terminal test suite.)

This patch will suppress stderr errors on tty in this situation.

Note that zsh does not allow to suppress execution of /etc/zshenv on
startup, so it's impossible to skip it in the test suite environment.

An alternative would be to set IN_NIX_SHELL in the test suite, but this
was rejected in upstream: kovidgoyal/kitty#7800

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
booxter added a commit to booxter/nix-darwin that referenced this pull request Aug 29, 2024
In some scenarios, the command may fail, e.g. when the shell is executed
with a different $HOME from where gpg agent is configured to run from.

(E.g. this happens in kitty terminal test suite.)

This patch will suppress stderr errors on tty in this situation.

Note that zsh does not allow to suppress execution of /etc/zshenv on
startup, so it's impossible to skip it in the test suite environment.

An alternative would be to set IN_NIX_SHELL in the test suite, but this
was rejected in upstream:

kovidgoyal/kitty#7800

There's also a kitty package specific fix posted here but this may be
unnecessary once nix-darwin is patched here:

NixOS/nixpkgs#338070

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants