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

Add __NIX_DARWIN_SET_ENVIRONMENT_DONE to default pass through envs (globalPassThroughEnv) #8701

Closed
1 task done
OliverJAsh opened this issue Jul 9, 2024 · 8 comments
Closed
1 task done
Labels
kind: bug Something isn't working owned-by: turborepo

Comments

@OliverJAsh
Copy link

OliverJAsh commented Jul 9, 2024

Verify canary release

  • I verified that the issue exists in the latest Turborepo canary release.

Link to code that reproduces this issue

N/A

What package manager are you using / does the bug impact?

Yarn v2/v3/v4 (node_modules linker only)

What operating system are you using?

Mac

Which canary version will you have in your reproduction?

N/A

Describe the Bug

When using nix-darwin with Turborepo's Strict Environment Variable Mode, $PATH is incorrectly overridden in child shells. For example:

package.json:

  "scripts": {
    "build": "echo $PATH && fish -c 'echo $PATH'"
  },

The second echo $PATH demonstrates that $PATH is being overridden by nix-darwin.

nix-darwin overrides the PATH for child shells if __NIX_DARWIN_SET_ENVIRONMENT_DONE is not set, as per the following code:

https://github.com/LnL7/nix-darwin/blob/fabc653517106127e2ed435fb52e7e8854354428/modules/environment/default.nix#L195-L198

Expected Behavior

nix-darwin can reliably detect child shells / $PATH is not overridden in child shells.

In order for it to do this, Turborepo should pass through the __NIX_DARWIN_SET_ENVIRONMENT_DONE environment variable, i.e. it should be added to the list of default pass through envs (globalPassThroughEnv) defined here: https://github.com/vercel/turbo/blob/ac6250d516c8f22251b92cd445906e8be48d8bc3/crates/turborepo-lib/src/task_hash.rs#L466-L470

To Reproduce

See above.

Additional context

If this sounds good I can raise a PR.

@OliverJAsh OliverJAsh added kind: bug Something isn't working needs: triage New issues get this label. Remove it after triage owned-by: turborepo labels Jul 9, 2024
@anthonyshew
Copy link
Contributor

Hey, thanks for the issue!

In the interest of being somewhat judicious about what we add to that list, I'd like to learn if this is a highly common use case in NixOS. I'm looking at their environment variables list and don't see the variable you're referring to. Is this something every NixOS user needs?

@OliverJAsh
Copy link
Author

OliverJAsh commented Jul 9, 2024

Thanks for the quick reply!

If I'm honest, I'm not too sure about the specifics. I only discovered this environment variable after hours of debugging why my $PATH was wrong inside child shells.

I imagine it's not listed there because it's an implementation detail—it's not something you're supposed to set yourself, rather it's set for you. I believe the issue only occurs when a tool like Turborepo prevents this environment variable from being passed down to a child shell.

cc my co-worker @samhh who may know better

@samhh
Copy link

samhh commented Jul 9, 2024

It comes from nix-darwin, not NixOS proper. This is the most relevant reference in the repo: https://github.com/LnL7/nix-darwin/blob/fabc653517106127e2ed435fb52e7e8854354428/modules/environment/default.nix#L195

I'd guess a significant minority of macOS Nix users use nix-darwin, and all of these will require this fix for subshells under Turbo.

@anthonyshew
Copy link
Contributor

That being the case, I'd recommend adding that variable to your repo's passthroughs. Alternatively, you could use Loose Mode for that invocation if you're still having trouble.

Our (admittedly non-scientific) criteria for adding to the default passthroughs is to capture the most vastly common, baseline cases for the most frequently used tools, so this case doesn't sound like it fits. In my thinking, the NIX_* namespace would be something we'd add but not __NIX_*.

Does that make sense? Hoping I explained that well enough and it tracks with the way NixOS works from what I'm gathering from your info here.

@samhh
Copy link

samhh commented Jul 9, 2024

That's fair. We're successfully using a global passthrough now absent an upstream solution. Maybe you could reconsider in the future if lots of other users hit the same issue, or if it never reaches critical mass at least it's documented here now.

My only qualm is that this feels similar to the anti-pattern of placing per-user configuration in per-repo VCS ignore files, but it'd be non-trivial to solve; I don't think Turbo has a global per-user config yet?

@anthonyshew anthonyshew removed the needs: triage New issues get this label. Remove it after triage label Jul 9, 2024
@anthonyshew
Copy link
Contributor

Going to close here given the above commentary.

@samhh, I'm interested, though. What do you mean by "global per-user config"?

@samhh
Copy link

samhh commented Jul 15, 2024

What do you mean by "global per-user config"?

Something like ~/.config/turbo/config.json. Global passthroughs there would be analogous to global VCS ignores in ~/.config/git/ignore.

@anthonyshew
Copy link
Contributor

Ah, I see. Yes, that'd certainly be interesting but not sure if we would roadmap it as it stands. This has been the first ask for this that I've heard!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working owned-by: turborepo
Projects
None yet
Development

No branches or pull requests

3 participants