treewide: Remove uses of builtins.toPath. #40937
Conversation
toPath has confusing semantics and is never necessary; it can always either just be omitted or replaced by pre-concatenating `/.`. It has been marked as "!!! obsolete?" for more than 10 years in a C++ comment, hopefully removing it will let us properly deprecate and, eventually, remove it.
|
I entirely agree. The semantics of |
| @@ -35,7 +35,7 @@ let | |||
|
|
|||
| mkUniquePkgs = pkgs: fastUnique (a: b: a < b) # highlighting hack: > | |||
| # here we deal with those dummy packages needed for hyphenation filtering | |||
| (map (p: if lib.isDerivation p then builtins.toPath p else "") pkgs); | |||
| (map (p: if lib.isDerivation p then p.outPath else "") pkgs); | |||
There was a problem hiding this comment.
@vcunat Can you validate that this is the right thing here?
There was a problem hiding this comment.
It needs something that "gets converted" to the store path and is reasonably ordered by < operator. I think .outPath should work.
|
+1 - same for Nix itself (and the docs). |
| # Abuse `builtins.toPath` to collapse possible double slashes | ||
| repoTag0 = builtins.toString (builtins.toPath "/${stripScheme registry}/${repository}/${imageName}"); | ||
| # Abuse paths to collapse possible double slashes | ||
| repoTag0 = builtins.toString (/. + "/${stripScheme registry}/${repository}/${imageName}"); |
There was a problem hiding this comment.
I think this can just be done with replaceStrings:
repoTag0 = replaceStrings ["//"] ["/"] "/${stripScheme registry}/${repository}/${imageName}";
There was a problem hiding this comment.
Isn't replaceStrings quite inefficient? Anyway trying to keep the changes minimal for this PR.
|
@edolstra Would like your feedback here, that we actually do want to deprecate toPath? |
|
Removing |
|
Sure, but we could stop using it, deprecate it with a warning, and possibly eventually replace it with an error saying "use --backwards-compat flag if you're trying to evaluate old code" or similar. |
toPath has confusing semantics and is never necessary; it can always
either just be omitted or replaced by pre-concatenating
/.. It hasbeen marked as "!!! obsolete?" for more than 10 years in a C++
comment, hopefully removing it will let us properly deprecate and,
eventually, remove it.