-
-
Notifications
You must be signed in to change notification settings - Fork 300
treewide: remove redundant stylix.image escaping and string coercion #2134
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
base: master
Are you sure you want to change the base?
treewide: remove redundant stylix.image escaping and string coercion #2134
Conversation
MattSturgeon
left a comment
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.
Initial review from my phone, so hard to test or view full context.
| '' | ||
| ${lib.getExe' pkgs.imagemagick "convert"} \ | ||
| ${lib.escapeShellArg config.stylix.image} \ | ||
| ${config.stylix.image} \ |
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.
Maybe some of these removed escapeShellArg and toString serialisers were handling the "null" case?
Afaict, moving from apply to coercedTo shouldn't significantly change the final semantics, so if serialising nix→shell was needed before, it may still be.
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.
Maybe some of these removed
escapeShellArgandtoStringserialisers were handling the "null" case?
The CI failure can be locally reproduced with the "schemeless" testbeds:
nix eval .#testbed:alacritty:schemelessFor some reason, adding lib.escapeShellArg back to the palette generator arguments fixes the CI failures:
diff --git a/stylix/palette.nix b/stylix/palette.nix
index db340676..86ccedf7 100644
--- a/stylix/palette.nix
+++ b/stylix/palette.nix
@@ -81,7 +81,7 @@ in
default = pkgs.runCommand "palette.json" { } ''
${lib.getExe cfg.paletteGenerator} \
"${cfg.polarity}" \
- ${cfg.image} \
+ ${lib.escapeShellArg cfg.image} \
"$out"
'';
};Maybe the palette generator parses the stylix.image argument incorrectly.
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.
Some filepaths contain whitespace:
$ nix eval .#testbed:alacritty:schemeless
trace:
cfg.image is: /nix/store/7shj92cnnfcxbkqlhhsmv3m7v7k59ii3-three-bicycles/three bicycles.jpg
escapeShellArg cfg.image is: '/nix/store/7shj92cnnfcxbkqlhhsmv3m7v7k59ii3-three-bicycles/three bicycles.jpg'
NOTE: This means the escapeShellArg removed on this line was also not redundant.
IMO, a cleaner way to handle this kinda thing generally is using derivationArgs:
pkgs.runCommand "palette.json"
{
inherit (cfg) polarity image;
}
''
${lib.getExe cfg.paletteGenerator} \
"$polarity" \
"$image" \
"$out"
'';derivation args always get serialised into shell variables, so things like whitespace get handled automatically.
You can also use nativeBuildInputs to put paletteGenerator on the PATH during the build, so you don't need getExe.
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.
Some filepaths contain whitespace:
$ nix eval .#testbed:alacritty:schemeless trace: cfg.image is: /nix/store/7shj92cnnfcxbkqlhhsmv3m7v7k59ii3-three-bicycles/three bicycles.jpg escapeShellArg cfg.image is: '/nix/store/7shj92cnnfcxbkqlhhsmv3m7v7k59ii3-three-bicycles/three bicycles.jpg'NOTE: This means the
escapeShellArgremoved on this line was also not redundant.
This is intentionally being tested by
stylix/stylix/testbed/images.nix
Lines 9 to 26 in 2b727da
| light = | |
| let | |
| image = fetchurl { | |
| name = "three-bicycles.jpg"; | |
| url = "https://unsplash.com/photos/hwLAI5lRhdM/download?ixid=M3wxMjA3fDB8MXxhbGx8fHx8fHx8fHwxNzE2MzYxNDcwfA&force=true"; | |
| hash = "sha256-S0MumuBGJulUekoGI2oZfUa/50Jw0ZzkqDDu1nRkFUA="; | |
| }; | |
| # Create a path containing a space to test that `stylix.image` is | |
| # correctly quoted when used as a shell argument. We have to use a | |
| # directory as the parent because derivation names themselves cannot | |
| # contain spaces. | |
| directory = runCommandLocal "three-bicycles" { } '' | |
| mkdir "$out" | |
| cp ${image} "$out/three bicycles.jpg" | |
| ''; | |
| in | |
| "${directory}/three bicycles.jpg"; |
to catch these edge cases.
IMO, a cleaner way to handle this kinda thing generally is using derivationArgs:
pkgs.runCommand "palette.json" { inherit (cfg) polarity image; } '' ${lib.getExe cfg.paletteGenerator} \ "$polarity" \ "$image" \ "$out" '';derivation args always get serialised into shell variables, so things like whitespace get handled automatically.
You can also use
nativeBuildInputsto put paletteGenerator on the PATH during the build, so you don't needgetExe.
Yes, I just think no one bothered refactoring this until now. I added commit 3f6651a ("stylix/palette: use derivation arguments instead of string interpolation") to this PR.
MrSom3body
left a comment
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.
hyprlock works :)
835c1c7 to
949b4ec
Compare
949b4ec to
cb5ae97
Compare
Co-authored-by: Matt Sturgeon <[email protected]>
Coerce derivations to store paths to ensure non-null values are stringified store paths with valid string contexts. Fixes: 838df8b ("stylix: improve `stylix.image` type (nix-community#1414)") Co-authored-by: Matt Sturgeon <[email protected]>
Remove redundant shell escaping and string coercion as stylix.image is guaranteed to be a stringified store path with a valid string context and no special characters. Fixes: 3499e3e ("treewide: properly quote stylix.image when used as a shell argument")
cb5ae97 to
c4ae2ea
Compare
MattSturgeon
left a comment
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.
Otherwise LGTM
| '' | ||
| ${lib.getExe' pkgs.imagemagick "convert"} \ | ||
| ${lib.escapeShellArg config.stylix.image} \ | ||
| ${config.stylix.image} \ |
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.
This will fail for any image path that needs shell escaping, such as ones containing whitespace. Same issue as what you've already addressed in palette.json.
MrSom3body
left a comment
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.
LGTM hyprlock still works
Uh oh!
There was an error while loading. Please reload this page.