fix: preserve local direnv settings - #26122

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.74%. Comparing base (f9b7f34) to head (1e624b1).
⚠️ Report is 14 commits behind head on main.

@@           Coverage Diff            @@
##             main   #26122    +/-   ##
========================================
  Coverage   82.73%   82.74%            
========================================
  Files        1147     1147            
  Lines      449459   449767   +308     
  Branches   449459   449767   +308     
========================================
+ Hits       371864   372148   +284     
- Misses      54936    54947    +11     
- Partials    22659    22672    +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@kosiew addressed + added source_up_if_exists to handle nested .envrc situation

🚀
@VVKot
Thank you for your contribution.

There are 3 issues with this:

1. flakes are experimental & should a project should never assume that if nix is on PATH that flakes are used. This will fail for a number of users.
2. Why isn’t the flake.nix just reexporting a shell.nix such that stable Nix users can use nix?
3. Was removing the file all together considered an option since those know how to use direnv can just run echo "use nix" >.envrc && direnv allow? Is this file providing enough value to warrant the potential security issue? The alternative is putting the whole .envrc in the .gitignore which allows any user to customize this as they see fit.

Thanks @toastal, fair points. This PR is already merged, so I'll follow up.

You're right about the security issue: direnv allow approves only the top-level .envrc. That means .envrc.local (and parent .envrc files loaded through source_up_if_exists) run without being re-approved.

I agree with option 3: untrack .envrc and add it to .gitignore. That also fixes #26101, which only happened because .envrc became tracked. It removes the need for .envrc.local and the flake/nix assumption too. The docs can just say echo 'use flake' > .envrc && direnv allow. A shell.nix for stable Nix users also sounds good.

@VVKot, what are your thoughts on this?

Thanks for the callouts @toastal! @kosiew - I had a different line of thinking here.

(1) is actually a few different things. Yes, flakes are technically experimental. All major upstream projects have a flake by now though: nix, NixOS, home manager, hardware. nix-darwin says Flakes (Recommended for beginners). A few of the newer projects (devenv, cachix, lix) only use flakes for development, so there is definitely a precedent for that. In short, I'll continue to wait for the day when https://www.youtube.com/watch?v=wWgxmchHSEw becomes true.

should a project should never assume that if nix is on PATH that flakes are used. This will fail for a number of users

That one's more important - I did check that direnv enables flakes on invocation, but I didn't consider that folks may be running sufficient outdated nix binary that doesn't support the flakes in the first place. I can add an extra check!

(2) shell.nix isn't hermetic / reproducible in the way flakes are. The are also not composable - for example, we could end up with flakes in datafusion-contrib inheriting from the one here. Frankly, as a newer Nix user, I'm familiar with the flakes, so that's what I contributed / happy to continue maintaining. @toastal feel free to raise a PR for shell.nix - I'm not comfortable doing so as I don't use the Nix the old way on any of my machines / don't know it well. For example, after a quick skim flake-compat sounds interesting -- if incomplete -- way to still support shell.nix while having flake as a source of truth.

(3) Yes, see the issue attached / direnv guidance that I followed. I think it does provide value - as mentioned in the original PR, there is under-appreciated value in having things "just work". Ghostty maintainers say it well here https://ghostty.org/docs/config#zero-configuration-philosophy. As you've said, the security issue is "potential", and the attack vector is "someone is able to modify a file on disk on your machine" which sounds more concerning than lack of checks for loading nested .envrc.local. I've subscribed to the latest issue here direnv/direnv#556 to update the setup in DataFusion once it is resolved.

@kosiew on (3) specifically - happy to go with untrack .envrc and add it to .gitignore if you feel strongly. My preference would be to update the .envrc to check for flake support instead.