nixos/etc: Replace make-etc.sh with nix and bash#131102
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
nixos/modules/system/etc/etc.nix
Outdated
There was a problem hiding this comment.
target=${lib.escapeShellArg target}
dasJ
force-pushed
the
feat/rework-etc-2
branch
2 times, most recently
from
aceb8b0 to
87d2792
Compare
ctheune
left a comment
ctheune
left a comment
There was a problem hiding this comment.
So bash isn't my lingua france but I poked around a couple of edge cases I could think of and it seems fine. I saw the jq-based version talk about performance but I think this should be safe. Not sure whether escapeShellArgs is specifically problematic.
If you want I can try to come up with a test case for this to avoid future regression.
nixos/modules/system/etc/etc.nix
Outdated
There was a problem hiding this comment.
I had to think twice whether this is safe regarding whitespace/newlines in the src as well but it seems so:
$ mkdir asdfsda$'\n'asdf
$ mkdir asdfsda$'\n'asdf2
$ for x in asdfsda*; do echo ".$x"; done
.asdfsda
asdf
.asdfsda
asdf2
nixos/modules/system/etc/etc.nix
Outdated
There was a problem hiding this comment.
Just curious: do you have an editor with working fenced syntax highlighting for Nix?
There was a problem hiding this comment.
Yes, vim-nix with the PR that hasn't been merged yet
|
Excuse my question but from the commit message I can't quite follow: The commit message says that you replace a shell script with Nix and a sh but how is that any different and why do we want this? Perhaps you can expand on that in the commit message documenting how this improves anything? |
The main goal of this commit is to replace the rather fragile passing of multiple arrays which could break in cases like NixOS#130935. While I could have just added proper shell escaping to the variables being passed, I opted for the more painful approach of replacing the fragile and somewhat strange construct with the 5 bash lists. While there are currently no more problems present with the current approach (at least none that I know of), the new approach seems more solid and might get around problems that could arise in the future stemming from either the multiple-lists situation or from the absence of proper shell quoting all over the script.
|
@andir like this? |
Yes, now it all makes sense without doing a multi-level history dive :) |
|
I tested this on some of my machines. |
| mkdir -p "$out/etc" | ||
| ${concatMapStringsSep "\n" (etcEntry: escapeShellArgs [ | ||
| "makeEtcEntry" | ||
| etcEntry.source |
There was a problem hiding this comment.
I just came across a case where this broke something.
Imagine source being a path such as ./my-local-file then this function will call toString on it leading to /home/andi/somewhere/my-local-file in the eval. This breaks setups that put version managed files into the store.
This manifests in the perl script that sets up our /etc/ to "warn" during runtime. Example hydra job: https://hydra.nixos.org/build/151772282/nixlog/20
5 participants
Motivation for this change
Alternative implementation for #131035.
This change changes the data format from 5 arrays to generated bash code.
Also, strings in the code are properly quoted now.
Closes #130935
Closes #131035
Closes #108779
Closes #41241
Closes #41276
Things done
sandboxinnix.confon non-NixOS linux)simple.nixnix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)