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

windows: Don't load login shell environment #22681

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

vilgotf
Copy link
Contributor

@vilgotf vilgotf commented Jan 5, 2025

I'm consistently getting the following error on startup:

2025-01-05T14:45:43.4602865+01:00 [ERROR] SHELL environment variable is not assigned so we can't source login environment variables

Caused by:
    environment variable not found

The source function, load_login_shell_environment, assumes a UNIX environment and should therefore not be called on Windows. (Unless you are using git bash?)

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 5, 2025
@maxdeviant maxdeviant changed the title windows: don't load login shell environment windows: Don't load login shell environment Jan 5, 2025
@0xtimsb
Copy link
Contributor

0xtimsb commented Jan 5, 2025

I think a better way to handle this would be to manage it at the call site and handle imports.

In main.rs:

    if !stdout_is_a_pty() {
        app.background_executor()
            .spawn(async {
                #[cfg(unix)]
                {
                    load_shell_from_passwd().log_err();
                }
                load_login_shell_environment().log_err(); // move this in above block
            })
            .detach()
    };

In utils.rs:

#[cfg(unix)] // add it here
pub fn load_login_shell_environment() -> Result<()> {

@vilgotf vilgotf force-pushed the win-shell branch 4 times, most recently from c2d3cdf to a6b783c Compare January 5, 2025 18:40
@zed-industries-bot
Copy link

Warnings
⚠️

This PR is missing release notes.

Please add a "Release Notes" section that describes the change:

Release Notes:

- Added/Fixed/Improved ...

If your change is not user-facing, you can use "N/A" for the entry:

Release Notes:

- N/A

Generated by 🚫 dangerJS against ab6d211

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

Thank you.

Nice note about git bash, we might need to revisit it later, but it seems to be ok for now.

@SomeoneToIgnore SomeoneToIgnore self-assigned this Jan 7, 2025
@SomeoneToIgnore SomeoneToIgnore added this pull request to the merge queue Jan 7, 2025
Merged via the queue into zed-industries:main with commit bb6e805 Jan 7, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants