-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactor: use pkgs.writeShellApplication
#2
refactor: use pkgs.writeShellApplication
#2
Conversation
modules/default.nix
Outdated
{ | ||
NVIM_RPLUGIN_MANIFEST = "${config.neovim.build.rplugin}/rplugin.vim"; | ||
} | ||
// (cfg.env or {}); |
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.
The current code produces
export FOO="${FOO:-bar}"
while runtimeEnv
produces
FOO='bar'
export FOO
So this one is actually on purpose. The reason is that I had planned on submitting an upstream PR to allow appending rather than prepending to PATH, but guess I just forgot :/ |
Interesting, I didn't think of this use case before, but that makes a lot of sense. Would going back to the previous way of setting |
No, doing so is the equivalent of:
but when the second export hits, the damage has already been done by |
The first line
doesn't get added for me when When I'm building my neovim.drv with e092693, I get:
And starting ...sorry if this is coming across as trying to force this PR, just trying to figure out if something changed recently, that leads to different behavior for my build. |
Cool I will try it out when I get home! |
e092693
to
2c4fb98
Compare
Following #1, I had a look through other todo notes, and this would be my approach.
Tried it on my dots again.
Seems that only the formatting changed.(cfg.env
changed, see my comments)writeShellApplication already runs shellcheck for us: https://github.com/NixOS/nixpkgs/blob/52544c4a0a84552db4013a8858ade52eaf7ff0ca/pkgs/build-support/trivial-builders/default.nix#L243-L357