-
-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
pkgs.substitute
: Deprecate replacements
, introduce substitutions
that can handle spaces
#287364
Conversation
src = builtins.toFile "source" '' | ||
Hello world! | ||
''; | ||
replacements = [ |
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.
It's just a gut feeling, but I think people may look at the first thing in tests here and assume that's the way to do it. So I'd put these last to put the 'good' examples at the top.
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.
Now flipped it around to the top :)
Yo peter! | ||
''; | ||
}; | ||
|
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.
Hmm, I did expect some more tests for the new one here actually. But maybe I'm missing something?
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 think it's fine because of the 5 existing tests, 3 of them test legacy behavior that was never really intended in the first place 😅. Then there's 1 test for the intended legacy attribute and 1 for the new attribute.
I'd like to leave testing other substitute
arguments to future PRs (probably not by me admittedly).
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.
Generally looks good, and tests are always appreciated! Left some nits but I don't consider them blocking.
a87a605
to
56c533d
Compare
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.
✅
... with a few comments and suggestions.
lib.warnIf (args ? replacements && lib.isInOldestRelease 2405) '' | ||
pkgs.substitute: For "${name}", `replacements` is used, which is deprecated since it doesn't support arguments with spaces. Use `substitutions` instead: | ||
substitutions = [ ${deprecationReplacement} ];''; |
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.
Remind me: does this mean it will go off for NixOS unstable users who are already in 24.05, or it will go off once 24.11 is cut on (say) May 31st 2024?
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 wrote a comment as a reply which I think justifies being in its own issue: #291939
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.
For what it's worth, I think it's worth sending a message now to users in 24.05 pre-release that they ought to migrate. I find migration warnings that go off many months later to be really surprising and irritating. That's when I expect the warning to transition to an error.
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 was missing a crucial part, and indeed I think what you're describing is actually what's happening, check out #291939 (comment)
@@ -8,7 +8,13 @@ if test -n "$dir"; then | |||
mkdir -p $out/$dir | |||
fi | |||
|
|||
substitute $src $target $replacements | |||
substitutionsList=($replacements) |
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.
Perhaps add a little TODO to remove this once replacements is done away with?
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'll add a TODO in the Nix file at the warning site :)
Hello world! | ||
''; | ||
substitutions = [ | ||
"--replace-fail" |
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.
Unrelated to this patch: I think --replace-fail
is what most of the other uses of --replace
wanted to use. Asking to replace something that doesn't exist is the sign of a rotted replacement!
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.
Agreed! I'd rather not bother trying to verify those for now.
pkgs/test/substitute/default.nix
Outdated
# Not great that this works at all, but is supported | ||
replacements = | ||
"--replace-fail world paul"; |
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 agree with the comment's POV.
builder = ./substitute.sh; | ||
inherit (args) src; | ||
preferLocalBuild = true; | ||
allowSubstitutes = false; | ||
} // args // { replacements = args.replacements; }) | ||
} // args // lib.optionalAttrs (args ? substitutions) { | ||
substitutions = lib.escapeShellArgs args.substitutions; |
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.
Because this uses lib.escapeShellArgs
, does this imply that it will reject raw strings, unlike the previous replacements
semantics?
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.
Indeed. The error would be pretty poor if it's not.. I'm adding a better error message :)
pkgs/test/substitute/default.nix
Outdated
# Not great that this works at all, but is supported | ||
replacements = [ | ||
"--replace-fail world paul" | ||
]; |
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.
Yikes; in some ways, this seems worse than the previous test case. Since the actual test strings are the same, I did find it hard to discern the difference textually between this and the previous test case. Suggest perhaps highlighting that this one uses a list where the other uses a string? Or changing the test case data?
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.
Doing some minor adjustments to highlight this more
56c533d
to
ac23165
Compare
Before I force push again to rebase, here's the diff of my changes: (diff) |
Also: - Add tests - Treewide update - Improve docs
ac23165
to
cd5dc76
Compare
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.
Merge PR 🚢 🦅 ✅
Description of changes
Fixes #286961. This is very similar to #164662
Things done
Add a 👍 reaction to pull requests you find important.