-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
noice: init at 0.6 #27091
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
noice: init at 0.6 #27091
Conversation
}; | ||
|
||
configFile = optionalString (conf!=null) (builtins.toFile "config.def.h" conf); | ||
preBuild = optionalString (conf!=null) "cp ${configFile} config.def.h"; |
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.
If you care to, I believe this could be simplified to echo ${conf} > config.h
directly instead of creating an intermediate.
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.
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.
Could be that toFile
handles large text chunks better. Other than that I fail to see the benefit of an intermediate here. Could be lack of imagination on my part, ofc :)
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.
Also, I guess toFile
is guaranteed to be "dumber" and so may be preferred.
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.
I've played around with echo
but -- regardless of all the different kinds of quoting I tried -- somehow it didn't work out. Shouldn't the expression just be as follows?
preBuild = optionalString (conf!=null) "echo ${conf} > config.def.h";
conf
itself was originally read by readFile
in my overlays which should be a string. I guess there is a reason why toFile
is preferred.
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.
What about something like
optionalString (conf != null) ''echo "${conf}" > config.h''
I suppose a heredoc would also work, in cases where echo
wouldn't. Anyway, if you prefer an intermediate, that's fine.
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.
I've tried that and did some debugging. Looks like both echo ${conf}
and echo "${conf}"
are not correctly escaping the given config which contains characters like ">\
. Only a heredoc solves this. Given these problems I'll keep it as it is.
Motivation for this change
This is a small and fast TUI file browser.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)