vercel / turbo

Incremental bundler and build system optimized for JavaScript and TypeScript, written in Rust – including Turbopack and Turborepo.
https://turbo.build
MIT License
25.7k stars 1.76k forks source link

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

Closed OliverJAsh closed 2 weeks ago

OliverJAsh commented 2 weeks ago

Verify 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.

anthonyshew commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

Going to close here given the above commentary.

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

samhh commented 1 week ago

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 commented 1 week ago

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!