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.

Sign up for GitHub

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

Jump to bottom

Direnv environments are not picked up with either fish or nushell used #8633

Closed
1 task done
WeetHet opened this issue Feb 29, 2024 · 10 comments · Fixed by #13902
Closed
1 task done

Direnv environments are not picked up with either fish or nushell used #8633

WeetHet opened this issue Feb 29, 2024 · 10 comments · Fixed by #13902
Labels
defect [core label] help wanted Looking for help from the community terminal Feedback for terminal integration, shell commands, etc

Comments

@WeetHet
Copy link
Contributor

WeetHet commented Feb 29, 2024 •
edited
Loading

Current status

  • Fish fix is merged
  • Nushell fix is yet to be found
  • Should we also ship fixes for stuff like elvish/oil/etc?

Check for existing issues

  • Completed

Describe the bug / provide steps to reproduce it

The current way to pick up the environment as used in

zed/crates/project/src/project.rs

Lines 9366 to 9385 in dab886f

let output = smol::process::Command::new(&shell)
.args([
"-i",
"-c",
// What we're doing here is to spawn a shell and then `cd` into
// the project directory to get the env in there as if the user
// `cd`'d into it. We do that because tools like direnv, asdf, ...
// hook into `cd` and only set up the env after that.
//
// The `exit 0` is the result of hours of debugging, trying to find out
// why running this command here, without `exit 0`, would mess
// up signal process for our process so that `ctrl-c` doesn't work
// anymore.
// We still don't know why `$SHELL -l -i -c '/usr/bin/env -0'` would
// do that, but it does, and `exit 0` helps.
&format!("cd {dir:?}; echo {marker}; /usr/bin/env -0; exit 0;"),
])
.output()
.await
.context("failed to spawn login shell to source login environment variables")?;

does not work with neither fish nor nu shells. This is because direnv has some weird behaviour preventing it from evaluating. The dirty fix would be to force evaluation:

let additional_command = match PathBuf::from_str(shell.as_str()) {
    Ok(shell_path) if shell_path.file_name() == Some(OsStr::new("fish")) => {
        Some("eval (direnv export fish)")
    }
    Ok(shell_path) if shell_path.file_name() == Some(OsStr::new("nu")) => {
        Some("direnv export json | from json | default {} | load-env")
    }
    _ => None,
};

let command = match additional_command {
    Some(additional_command) => format!(
        "cd {dir:?}; {}; echo {marker}; /usr/bin/env -0; exit 0;",
        additional_command
    ),
    None => format!("cd {dir:?}; echo {marker}; /usr/bin/env -0; exit 0;"),
};

or something like that.

Environment

Zed: v0.125.1 (Zed Preview)
OS: macOS 14.3.1
Memory: 8 GiB
Architecture: aarch64

If applicable, add mockups / screenshots to help explain present your vision of the feature

Screenshot 2024-02-29 at 22 00 12

If applicable, attach your ~/Library/Logs/Zed/Zed.log file to this issue.

If you only need the most recent lines, you can run the zed: open log command palette action to see the last 1000.

No response
@WeetHet WeetHet added admin read Pending admin review defect [core label] triage Maintainer needs to classify the issue labels Feb 29, 2024
@WeetHet
Copy link
Contributor Author

WeetHet commented Feb 29, 2024

@mrnugget, this seems like your stuff

@Moshyfawn Moshyfawn mentioned this issue Mar 1, 2024
Open
1 task
@WeetHet
Copy link
Contributor Author

WeetHet commented Mar 1, 2024 •
edited
Loading

@mrnugget I have discovered a horrible hack for fish: add emit fish_prompt after cd, which triggers direnv
Screenshot 2024-03-01 at 10 16 57

@WeetHet
Copy link
Contributor Author

WeetHet commented Mar 1, 2024

Or do set -x __direnv_export_again; emit fish_preexec;
Screenshot 2024-03-01 at 10 15 35

@mrnugget
Copy link
Member

mrnugget commented Mar 1, 2024

Just to confirm this: you're running into this when trying to use gopls, right? Because no other place uses this method yet.

@WeetHet
Copy link
Contributor Author

WeetHet commented Mar 1, 2024 •
edited
Loading

Just to confirm this: you're running into this when trying to use gopls, right? Because no other place uses this method yet.

Yeah, but its a more general problem with some tools (like direnv) requiring fish_prompt event to activate

@mrnugget
Copy link
Member

mrnugget commented Mar 1, 2024

Got it. I think special-casing fish and using emit fish_prompt after cd isn't that hacky. Why do you think it's hacky? What am I missing?

@WeetHet
Copy link
Contributor Author

WeetHet commented Mar 1, 2024

Got it. I think special-casing fish and using emit fish_prompt after cd isn't that hacky. Why do you think it's hacky? What am I missing?

We need to do that for every shell, I still don't know what should be done for nushell

@WeetHet
Copy link
Contributor Author

WeetHet commented Mar 1, 2024

We can do fish for now and leave this open for suggestions on nushell

@WeetHet WeetHet mentioned this issue Mar 1, 2024
Merged
WeetHet added a commit to WHForks/zed that referenced this issue Mar 1, 2024
@WeetHet
Add a fish-specific fix for zed-industries#8633
290a94a
@Moshyfawn Moshyfawn added terminal Feedback for terminal integration, shell commands, etc and removed triage Maintainer needs to classify the issue labels Mar 2, 2024
mrnugget added a commit that referenced this issue Mar 4, 2024
@WeetHet @mrnugget
Add a fish-specific fix for #8633 (#8659)
38f106c 
Release Notes:
- Fixed detection of `direnv` not working in `fish` when an LSP adapter
(`gopls`, for example) tries to detect user-installed binaries. (#8633)

---------

Co-authored-by: Thorsten Ball <[email protected]>
@JosephTLyons JosephTLyons removed the admin read Pending admin review label Mar 4, 2024
@JosephTLyons
Copy link
Contributor

This is available in Zed v0.126.0-pre.

@WeetHet
Copy link
Contributor Author

WeetHet commented Mar 6, 2024

@JosephTLyons please reopen as this is still not implemented for nushell. Also labeling this as help needed would be great as I have no idea how to fix nushell

@JosephTLyons JosephTLyons added the help wanted Looking for help from the community label Mar 6, 2024
@JosephTLyons JosephTLyons reopened this Mar 6, 2024
@Moshyfawn Moshyfawn mentioned this issue Mar 7, 2024
Open
1 task
@WeetHet WeetHet mentioned this issue Jul 6, 2024
Merged
SomeoneToIgnore pushed a commit that referenced this issue Jul 15, 2024
@WeetHet @mrnugget @SomeoneToIgnore
Fix nushell local env detection by using direnv export (#13902)
efcd2a5 
I don't intend fully on getting this merged, this is just an experiment
on using `direnv` directly without relying on shell-specific behaviours.
It works though, so this finally closes #8633
Release Notes:

- Fixed nushell not picking up `direnv` environments by directly
interfacing with it using `direnv export`

---------

Co-authored-by: Thorsten Ball <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
defect [core label] help wanted Looking for help from the community terminal Feedback for terminal integration, shell commands, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix nushell local env detection by using direnv export WHForks/zed
4 participants
@mrnugget @Moshyfawn @JosephTLyons @WeetHet