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

Direnv sets a terminfo variable to a string, when in zsh it's an array #405

Closed
acid-bong opened this issue Nov 13, 2023 · 11 comments · Fixed by #461
Closed

Direnv sets a terminfo variable to a string, when in zsh it's an array #405

acid-bong opened this issue Nov 13, 2023 · 11 comments · Fixed by #461

Comments

@acid-bong
Copy link

acid-bong commented Nov 13, 2023

I've created a flake for my build of st terminal, and originally it was overriding some attributes of nixpkgs' derivation. In that case direnv (with use flake in .envrc) didn't load the environment consistently:

  • sometimes it didn't load the executables: make, pkg-config and/or gcc
  • sometimes it didn't load the libraries
  • sometimes it didn't set up some variables (such as CC=gcc)
  • Starship's nix module didn't display fully: either had only impure, no text or kept the name of different flakes I maintain (I cd in and out my flakes), and sometimes it kept the word impure when I exited the troublesome flake dir

It was also noticeable that it tried to run the preInstall command defined by nixpkgs' derivation of st, and TERMINFO variable didn't go well with Zsh (I assume Direnv loads asynchronously) -- it produced (eval):export:1: terminfo: inconsistent type for assignment (although tried setting it manually, works normally). Retried entering and exiting in Bash, no such issue appears.

I rewrote the flake from overrideAttrs and basically copypasted the build recipe from nixpkgs, now Direnv loads the environment consistently, doesn't run that export command.

This also didn't happen in other flakes, like dwm (also overrideAttrs-based) and manually patched Victor Mono (mkDerivation-based).

@bbenne10
Copy link
Contributor

I keep coming back to this, trying to respond, and then closing the tab because there's just a lot going on and formulating a response is frankly a bit difficult.

I think the reason for this is that it is unclear where to start debugging. Are you sure you had nix-direnv loaded? (direnv includes a non-caching use nix and use flake implementation. I'm not pointing fingers, but I can't speak for their implementation at all) If you are sure, what version of nix-direnv are you using to replicate the behavior? What does your .envrc look like? Are you overriding your layout directory (if you don't know the answer to this, you are not.)

To clarify your point about direnv loading async: It does not. direnv kicks in when your prompt is going to be generated. It launches a subshell wherein it runs your .envrc (and use flake, for instance, runs a function called use_flake and proxies any arguments along, which is where nix-direnv gets involved). The subshell it runs is always bash. It launches the subshell, collects the environment, runs any code, and then collects and diffs the environment with the first collection. The net result is applied to the calling shell, which is why there's some oddities surrounding what can be exported. Notably - functions cannot be shared between shells. This is all to explain how this works - not to necessarily point at any point as broken.

looking at your flakes: I would expect src = ./. (or filterSource or something else in there too), but I may just not be familiar with this pattern. If you're saying one works and the other does not, I trust that this is not the problem though.

I might suggest checking on the profile rc that is created during nix-direnv's evaluation (and which is the thing we use for caching). Maybe see if the profile_rc's created by the two flakes are equivalent or if the overrideAttrs based derivation differs signficantly? I would expect them to functionally be the same.

@bbenne10
Copy link
Contributor

bbenne10 commented Jan 2, 2024

ping? I don't want to close this without addressing it, but without feedback I don't know that we can do much?

@acid-bong
Copy link
Author

Sorry, I went with copying the entire mkDerivation and eventually forgot about this

@Mic92 Mic92 closed this as completed Jan 2, 2024
@acid-bong
Copy link
Author

acid-bong commented Jan 3, 2024

Goddamit, I only now understood the actual issue: zsh doesn't like when a package has terminfo output and direnv tries to assign a value to $terminfo (which is an array in zsh). Should we reopen (and rename) this issue or file a new one? Or it's specific to direnv and not nix-direnv and belongs to their tracker?

Late edit: it's actually resolved, adding unset terminfo to .envrc was sufficient

@bbenne10
Copy link
Contributor

bbenne10 commented Jan 3, 2024

Okay - I don't really think this is nix-direnv's problem, as we faithfully recreated the environment we were handed. But I also do not like leaving it as "well, just unset problematic variables". @Mic92 Do you have ideas on how we might blacklist some variables for some host shells? I don't think we can rely on $SHELL as it should always be set to bash inside direnv.

For anyone else watching: We had a short discussion on matrix about this and I asked about the feasibility of adding a host_shell variable to the shell hooks sourced by all direnv users. This seems intuitive, but maybe also a bit drastic because we would be again waiting on new direnv functionality.

@Mic92 Mic92 reopened this Jan 4, 2024
@Mic92
Copy link
Member

Mic92 commented Jan 4, 2024

We do filter some environment variables out:

_nix_export_or_unset NIX_BUILD_TOP "$old_nix_build_top"

Question is, is terminfo worth it being treated special? I would expect very few derivations would try to export this. The work-around is adding unset terminfo to either shellHook or .envrc.

@bbenne10
Copy link
Contributor

bbenne10 commented Jan 4, 2024

I think we can achieve this simply?

diff --git a/direnvrc b/direnvrc
index 50a84cf..24a7efb 100644
--- a/direnvrc
+++ b/direnvrc
@@ -137,6 +137,7 @@ _nix_import_env() {
   local old_temp=${TEMP:-__UNSET__}
   local old_tempdir=${TEMPDIR:-__UNSET__}
   local old_xdg_data_dirs=${XDG_DATA_DIRS:-}
+  local old_terminfo=${terminfo:-__UNSET__}
 
   # On the first run in manual mode, the profile_rc does not exist.
   if [[ ! -e $profile_rc ]]; then
@@ -158,6 +159,7 @@ _nix_import_env() {
   _nix_export_or_unset TMPDIR "$old_tmpdir"
   _nix_export_or_unset TEMP "$old_temp"
   _nix_export_or_unset TEMPDIR "$old_tempdir"
+  _nix_export_or_unset TERMINFO "$old_terminfo"
   local new_xdg_data_dirs=${XDG_DATA_DIRS:-}
   export XDG_DATA_DIRS=
   local IFS=:

But I might be a bit more aggressive here (bash introduced associative arrays in 4, so this is inline with our 4.4 minimum):

diff --git a/direnvrc b/direnvrc
index 50a84cf..dbd97d1 100644
--- a/direnvrc
+++ b/direnvrc
@@ -131,12 +131,15 @@ _nix_export_or_unset() {
 _nix_import_env() {
   local profile_rc=$1
 
-  local old_nix_build_top=${NIX_BUILD_TOP:-__UNSET__}
-  local old_tmp=${TMP:-__UNSET__}
-  local old_tmpdir=${TMPDIR:-__UNSET__}
-  local old_temp=${TEMP:-__UNSET__}
-  local old_tempdir=${TEMPDIR:-__UNSET__}
-  local old_xdg_data_dirs=${XDG_DATA_DIRS:-}
+  local -A values_to_restore=(
+    ["NIX_BUILD_TOP"]=${NIX_BUILD_TOP:-__UNSET__},\
+    ["tmp"]=${TMP:__UNSET__},\
+    ["TMPDIR"]=${TMPDIR:-__UNSET__},\
+    ["TEMP"]=${TEMP:-__UNSET__},\
+    ["TEMPDIR"]=${TEMPDIR:-__UNSET__},\
+    ["terminfo"]=${terminfo:-__UNSET__}
+  )
+  local old_xdg_data_dirs=${"XDG_DATA_DIRS":-}
 
   # On the first run in manual mode, the profile_rc does not exist.
   if [[ ! -e $profile_rc ]]; then
@@ -153,11 +156,10 @@ _nix_import_env() {
     rm -rf "$NIX_BUILD_TOP"
   fi
 
-  _nix_export_or_unset NIX_BUILD_TOP "$old_nix_build_top"
-  _nix_export_or_unset TMP "$old_tmp"
-  _nix_export_or_unset TMPDIR "$old_tmpdir"
-  _nix_export_or_unset TEMP "$old_temp"
-  _nix_export_or_unset TEMPDIR "$old_tempdir"
+  for key in ${!values_to_restore[@]}; do
+    _nix_export_or_unset "$key" "${values_to_restore[${key}]}"
+  done
+
   local new_xdg_data_dirs=${XDG_DATA_DIRS:-}
   export XDG_DATA_DIRS=
   local IFS=:

@Mic92
Copy link
Member

Mic92 commented Jan 5, 2024

Yeah. now that we allow bash4 we can use associative arrays.

@bbenne10
Copy link
Contributor

bbenne10 commented Jan 5, 2024

I'll get a PR open soon for this. I have the work done already locally.

@bbenne10
Copy link
Contributor

bbenne10 commented Jan 5, 2024

@acid-bong: Could you please give this a test with latest master? Should be resolved now. :)

@acid-bong
Copy link
Author

acid-bong commented Jan 6, 2024

Can confirm, it does work now as expected

@acid-bong acid-bong changed the title Direnv behaves differently, when flakes have identical result Direnv sets a terminfo variable to a string, when in zsh it's an array Apr 8, 2024
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 a pull request may close this issue.

3 participants