lib: add isInStore and isPath, slighly change isStorePath #31715
Conversation
lib/strings.nix
Outdated
| */ | ||
| isInStore = s: | ||
| (builtins.isString s || isPath s) | ||
| && (builtins.substring 0 (builtins.stringLength builtins.storeDir) s) == builtins.storeDir; |
There was a problem hiding this comment.
Could use hasPrefix builtins.storeDir s here, I think.
- isStorePath now returns true for paths - isPath returns true only when the type of value is path - isInStore just asserts that the path is prefixed with store dir
domenkozar
force-pushed
the
lib+paths
branch
from
7897128
to
05f5ab7
Compare
| @@ -219,8 +219,7 @@ rec { | |||
|
|
|||
| path = mkOptionType { | |||
| name = "path"; | |||
| # Hacky: there is no ‘isPath’ primop. | |||
| check = x: builtins.substring 0 1 (toString x) == "/"; | |||
| check = x: isString x || isPath x; | |||
There was a problem hiding this comment.
I'm wondering whether this was intended to be isString x && isPath x. As-is, wouldn't some nonsense value like "$%^&*" be accepted, as long as it is a string?
There was a problem hiding this comment.
As-is, wouldn't some nonsense value like "$%^&*" be accepted, as long as it is a string?
Yeah, that seems to be the case. Is that intended?
There was a problem hiding this comment.
I don't see why arbitrary strings should be accepted as a path.
There was a problem hiding this comment.
Yes, this needs to allow (string or path type) paths.
There was a problem hiding this comment.
It would need to be (isString x && is absolute) || isPath x. Will try to fix that.
| @@ -438,13 +438,53 @@ rec { | |||
| => true | |||
| isStorePath pkgs.python | |||
| => true | |||
| isStorePath ./types.nix | |||
| => true | |||
There was a problem hiding this comment.
That should have always been handled like this. I’d count it as a bug fix.
| @@ -429,7 +429,7 @@ rec { | |||
| */ | |||
| fixedWidthNumber = width: n: fixedWidthString width "0" (toString n); | |||
|
|
|||
| /* Check whether a value is a store path. | |||
| /* Check whether a string or a path is a store path. | |||
There was a problem hiding this comment.
It’s still a value, since we’re returning a bool for every possible value. I’d change the docs to:
/* Check whether a value is a store path.
Every literal path (e.g. `./foobar`) is a store path,
every string that points to a file directly under `buildins.storeDir` as well.
*/
| && builtins.substring 0 1 (toStringContext x) == "/" | ||
| && dirOf (toStringContext x) == builtins.storeDir; | ||
|
|
||
| /* Check whether a string or a path is in the store. |
There was a problem hiding this comment.
Same as above, it’s really a check for any value. Otherwise you have to omit the isString/isPath check.
| @@ -219,8 +219,7 @@ rec { | |||
|
|
|||
| path = mkOptionType { | |||
| name = "path"; | |||
| # Hacky: there is no ‘isPath’ primop. | |||
| check = x: builtins.substring 0 1 (toString x) == "/"; | |||
| check = x: isString x || isPath x; | |||
There was a problem hiding this comment.
As-is, wouldn't some nonsense value like "$%^&*" be accepted, as long as it is a string?
Yeah, that seems to be the case. Is that intended?
| isInStore "/home/me/whatever.nix" | ||
| => false | ||
| isInStore ./types.nix | ||
| => true |
There was a problem hiding this comment.
This does not make sense, given that toString ./types.nix returns /local/directory/types.nix.
There was a problem hiding this comment.
In other words, this function doesn't test whether something is in the store but whether it can be coerced to a store path in certain contexts (such as in a string anti-quotation)..
There was a problem hiding this comment.
Same could be argued that strings lose their context. It does coerce it to string (and thus import it to store), but it doesn't mean it can't be converted to another type which gets another meaning.
The nonStorePath type is the more interesting of the two; it should help prevent secrets from being written to the store inadvertently. Note that this is based somewhat on the proposal here: NixOS/nixpkgs#31715 but is a bit less ambitious.
|
What is the status of this pull request? |
|
Closing due to lack of activity, feel free re-open this if needed. |
Motivation for this change
Groundwork for #24288 (comment)
This adds better tooling to differentiate between string and path types. Also allows to see if path is in nix store or just a regular one.
I plan to add docs once we agree this is a good idea.