-
-
Notifications
You must be signed in to change notification settings - Fork 16.3k
nixos/direnv: fix silent option... again #402399
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it suddenly stop working? Is makeBinaryWrapper
broken? Does it work with makeShellWrapper
?
And, could we also add a test for this?
not a clue, direnv wasn't updated, nix-direnv wasn't updated, the module wasn't touched... |
Oh, I see the problem. So now there's two direnvs floating around, the shell hook is unwrapped but the direnv on Rather than overriding |
@eclairevoyant pushed you suggested changes, still isn't working. i think we have to set |
i kept the finalPackage machinery, set |
6f9d461
to
3c61c7e
Compare
So I was investigating this on my own because I didn't find this PR. |
That's basically what I said in #402399 (comment), unless you're saying something else. Or maybe better as a question: what code are you referring to? |
The output of |
Oh, okay, what I was saying was different then. It seems like it's because of using |
Hmm actually I guess the problem will still arise because the wrapped program would call the unwrapped one, thereby introducing unwrapped code as part of the hook? We'd need some sort of "wrap-propagation" which I can't think of a clean way to do. I guess wrapping is just the wrong approach, and we should stick with the PR as written here. EDIT: even |
@eclairevoyant I tried and you are right, the problem is that we use
Then it correctly uses the final build path in |
Direnv is a fairly small build, so the reduced environmental pollution is probably worth the build, I'll test it then change this PR tonight |
Oh, I see, the problem with this is that direnv will need to be rebuilt. |
|
Strange. I did this inside
The resulting package has the correct direnv in |
By the way, I think that the approach that is currently in this PR should be merged, because it works (unlike what is in master right now) and improvements can be made after that. |
True, but I think none of us (?) are committers |
Some interesting ideas for future PRs in #404692 btw |
You are right, DIRENV_CONFIG is not being set with what I wrote, it was set from my global environment. Sorry for misleading. I don't know why wrapping in |
I have this issue on a freshly updated 25.05. Edit: I can confirm that using this fix worked for me on 25.05. |
Successfully created backport PR for |
wrapping direnv with
DIRENV_CONFIG
was working... until it stopped.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.