Skip to content
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

toString on path value in store drops string context #8428

Open
roberth opened this issue Jun 1, 2023 · 2 comments
Open

toString on path value in store drops string context #8428

roberth opened this issue Jun 1, 2023 · 2 comments
Labels
documentation error-messages Confusing messages and better diagnostics language The Nix expression language; parser, interpreter, primops, evaluation, etc

Comments

@roberth
Copy link
Member

roberth commented Jun 1, 2023

Describe the bug

toString on a path value that references a store path does not create a string context.

I suppose path values weren't designed to contain store paths, but flakes do it. I don't know how relevant this is after lazy trees, but it can still be encountered with import "${./.}" if default.nix references relative paths.

Steps To Reproduce

builtins.getContext (toString /nix/store/aaxda5ywpsjwr1j9fkrrks1p0iyaxg97-source)
{ }

or for a realistic example, #7796 (comment)

     runCommand "bad" { path = toString ./.; } ''
       echo "$path"
       ls "$path" || echo "./. is not a proper path and is thus not accessible"
     '';

Expected behavior

toString ./. preserves string context when ./. is in the store?

On the other hand, toString /var does not and can not have a string context, so perhaps we shouldn't project this expectation onto toString.

nix-env --version output

Additional context

Add any other context about the problem here.

Priorities

Add 👍 to issues you find important.

@roberth roberth added bug language The Nix expression language; parser, interpreter, primops, evaluation, etc labels Jun 1, 2023
dixslyf added a commit to dixslyf/haskell-flake that referenced this issue Jun 15, 2023
Before this commit, the type of a package source (haskell-source-type)
depended on string contexts in its `check` function. However, string
contexts do not work well to represent a path to a package source. For
instance, "hello ./subdirectory" would have a string context, but is
obviously not a valid path.

Furthermore, a `nix` bug (NixOS/nix#8428)
related to string contexts triggers an issue where setting the project
root causes a type error with haskell-source-type (srid#169).

This commit replaces haskell-source-type with `with type; either path str`
and instances of `isPathUnderNixStore` with `lib.types.path.check` to
avoid using string contexts.
srid added a commit to srid/haskell-flake that referenced this issue Jun 15, 2023
* fix: don't rely on string context for package source

Before this commit, the type of a package source (haskell-source-type)
depended on string contexts in its `check` function. However, string
contexts do not work well to represent a path to a package source. For
instance, "hello ./subdirectory" would have a string context, but is
obviously not a valid path.

Furthermore, a `nix` bug (NixOS/nix#8428)
related to string contexts triggers an issue where setting the project
root causes a type error with haskell-source-type (#169).

This commit replaces haskell-source-type with `with type; either path str`
and instances of `isPathUnderNixStore` with `lib.types.path.check` to
avoid using string contexts.

* Update CHANGELOG.md

---------

Co-authored-by: Sridhar Ratnakumar <3998+srid@users.noreply.github.com>
@sternenseemann
Copy link
Member

toString ./. preserves string context when ./. is in the store?

This would be extremely confusing. The whole point of toString is to convert paths into strings without incurring any kind of string context, while "${path}" preserves it (albeit by re-dumping the path even if it is in store). I could see an argument for treating the path specially depending on whether it is already in builtins.storeDir or not in the latter case, but not in the former.

@roberth
Copy link
Member Author

roberth commented Jul 14, 2023

On the other hand, toString /var does not and can not have a string context, so perhaps we shouldn't project this expectation onto toString.

[changing] This would be extremely confusing.

I think this is the right interpretation.
Path values simply don't have a string context, until you copy them using string interpolation / concatenation.

So then this would be a documentation and/or error messages issue; not a bug.

We should perhaps discourage the use of

/. + someStorePathString

and #6530 should also help prevent this, to the degree that flake copying to the store is the problem here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation error-messages Confusing messages and better diagnostics language The Nix expression language; parser, interpreter, primops, evaluation, etc
Projects
None yet
Development

No branches or pull requests

2 participants