Add support for "path:./relative/path" #18

Closed

BBBSnowball wants to merge 3 commits from BBBSnowball/pr-1 into master

BBBSnowball commented

2021-01-20 01:45:56 +00:00

(Migrated from github.com)

My system config flake uses inputs.private.url = "path:./private"; for information that is ok to be in the store but shouldn't be on Github. This is a git submodule so a relative path feels like the right way to refer to it.

Without this patch, flake-compat wants an absolute path:

# nix --version
nix (Nix) 2.4pre20201201_5a6ddb3

# nix-build '<nixpkgs/nixos>' -A system
warning: Git tree '/etc/nixos' is dirty
error: --- EvalError ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- nix-build
at: (42:35) in file: /nix/store/2mpgi4bvn8py4liv9w3mjxd2c5r7bvv8-source/default.nix

    41|     else if info.type == "path" then
    42|       { outPath = builtins.path { path = info.path; };
      |                                   ^
    43|         narHash = info.narHash;

string './private' doesn't represent an absolute path
(use '--show-trace' to show detailed location information)

My system config flake uses `inputs.private.url = "path:./private";` for information that is ok to be in the store but shouldn't be on Github. This is a git submodule so a relative path feels like the right way to refer to it. Without this patch, flake-compat wants an absolute path: ``` # nix --version nix (Nix) 2.4pre20201201_5a6ddb3 # nix-build '<nixpkgs/nixos>' -A system warning: Git tree '/etc/nixos' is dirty error: --- EvalError ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- nix-build at: (42:35) in file: /nix/store/2mpgi4bvn8py4liv9w3mjxd2c5r7bvv8-source/default.nix 41| else if info.type == "path" then 42| { outPath = builtins.path { path = info.path; }; | ^ 43| narHash = info.narHash; string './private' doesn't represent an absolute path (use '--show-trace' to show detailed location information) ```

immae (Migrated from github.com) reviewed 2021-01-24 00:07:15 +00:00

immae (Migrated from github.com) left a comment

A more generic solution would allow for any relative path (possibly parents, for instance, which are accepted by flakes too) and not just './':

          if builtins.substring 0 1 info.path == "."                                                                   
          then builtins.toString src + "/" + info.path
          else info.path;

A more generic solution would allow for any relative path (possibly parents, for instance, which are accepted by flakes too) and not just './': ``` if builtins.substring 0 1 info.path == "." then builtins.toString src + "/" + info.path else info.path; ```

BBBSnowball commented

2021-01-24 00:59:18 +00:00

(Migrated from github.com)

I like the idea, done in d1a6788892

However, this would cause inconsistent behavior for "relative/path" vs. ".hidden/path", right? Should we rather test with if builtins.substring 0 1 info.path != "/"? We don't have any isAbsolutePath in builtins, it seems.

nixpkgs seems to be doing the same, e.g. here: 78782d368e/lib/sources.nix (L131)

Are there any use cases that may be broken by this?

I like the idea, done in https://github.com/BBBSnowball/flake-compat/commit/d1a6788892881f2d15836d0ac1849d844d7b01de However, this would cause inconsistent behavior for `"relative/path"` vs. `".hidden/path"`, right? Should we rather test with `if builtins.substring 0 1 info.path != "/"`? We don't have any `isAbsolutePath` in builtins, it seems. nixpkgs seems to be doing the same, e.g. here: https://github.com/NixOS/nixpkgs/blob/78782d368e91d0b2814b9bb2f343353fe8583e01/lib/sources.nix#L131 Are there any use cases that may be broken by this?

immae commented

2021-01-24 09:57:48 +00:00

(Migrated from github.com)

However, this would cause inconsistent behavior for "relative/path" vs. ".hidden/path", right? Should we rather test with if builtins.substring 0 1 info.path != "/"? We don't have any isAbsolutePath in builtins, it seems.

Ah yes I forgot about hidden paths (isn’t there a story about that being the initial reason why paths starting with "." were hidden ? 😁 ), checking for "/" as first char is fine :)

> However, this would cause inconsistent behavior for `"relative/path"` vs. `".hidden/path"`, right? Should we rather test with `if builtins.substring 0 1 info.path != "/"`? We don't have any `isAbsolutePath` in builtins, it seems. Ah yes I forgot about hidden paths (isn’t there a story about that being the initial reason why paths starting with "." were hidden ? :grin: ), checking for "/" as first char is fine :)

BBBSnowball commented

2021-01-24 16:51:02 +00:00

(Migrated from github.com)

isn’t there a story about that being the initial reason why paths starting with "." were hidden ? grin

I didn't know that. Something like "let's hide . and .." and they were hiding .hidden, as well? If you ever find that story again, please send me a link. :)

checking for "/" as first char is fine :)

Done.

> isn’t there a story about that being the initial reason why paths starting with "." were hidden ? grin I didn't know that. Something like "let's hide `.` and `..`" and they were hiding `.hidden`, as well? If you ever find that story again, please send me a link. :) > checking for "/" as first char is fine :) Done.

immae commented

2021-01-24 18:01:31 +00:00

(Migrated from github.com)

I didn't know that. Something like "let's hide . and .." and they were hiding .hidden, as well? If you ever find that story again, please send me a link. :)

This kind of story:

https://linux-audit.com/linux-history-how-dot-files-became-hidden-files/

Note that although it seems quite a plausible story I never went to check that it was actually true (there were probably control version at that time, so it’s probably easy to check if it’s true though :p )

> I didn't know that. Something like "let's hide . and .." and they were hiding .hidden, as well? If you ever find that story again, please send me a link. :) This kind of story: https://linux-audit.com/linux-history-how-dot-files-became-hidden-files/ Note that although it seems quite a plausible story I never went to check that it was actually true (there were probably control version at that time, so it’s probably easy to check if it’s true though :p )

BBBSnowball commented

2021-01-24 19:53:21 +00:00

(Migrated from github.com)

Thanks :)

jorsn commented

2021-02-26 12:21:53 +00:00

(Migrated from github.com)

  // FIXME: check whether access to 'path' is allowed.

So, this will probably have to be changed in the future again. Nevertheless, this PR adjusts flake-compat to current flake behavior, so it would be great to merge this, @edolstra.

If I understand correctly, this now allows for completely arbitrary paths, doesn't it? This is the current behavior of real flakes. As flake-compat should mimic real flake behavior, note [this comment](https://github.com/NixOS/nix/blob/6548b89cc4eb214cb4632fd4332c610f2d1f0a9d/src/libfetchers/path.cc#L87) in the path fetcher: ~~~ // FIXME: check whether access to 'path' is allowed. ~~~ So, this will probably have to be changed in the future again. Nevertheless, this PR adjusts flake-compat to current flake behavior, so it would be great to merge this, @edolstra.

BBBSnowball commented

2021-02-26 22:53:49 +00:00

(Migrated from github.com)

If I understand correctly, this now allows for completely arbitrary paths, doesn't it?

Yes. The old code allows arbitrary absolute paths. This PR adds support for arbitrary relative paths.

// FIXME: check whether access to 'path' is allowed.

Do you know which paths are not allowed? I thought any path would be ok: Store paths are passed as-is and other paths are copied to the store. If this is not so, I should probably adjust my flakes sooner than later. I'm using lots of relative paths.

So, this will probably have to be changed in the future again.

We may want to forgo this PR or adjust it if this change is landing soon-ish, e.g. documentation exists and/ or code exists in some branch. If I know what to change, I can adjust the PR.

On a second thought: I think flake-compat is already more lenient than real flakes. For example, I had to change some code that was building temporary paths to /etc, which obviously isn't allowed in a real flake (but flake-compat cannot check for this). We might be ok with flake-compat being simple and a superset of (rather than identical to) real flakes, right?

> If I understand correctly, this now allows for completely arbitrary paths, doesn't it? Yes. The old code allows arbitrary absolute paths. This PR adds support for arbitrary relative paths. > `// FIXME: check whether access to 'path' is allowed.` Do you know which paths are not allowed? I thought any path would be ok: Store paths are passed as-is and other paths are copied to the store. If this is not so, I should probably adjust my flakes sooner than later. I'm using lots of relative paths. > So, this will probably have to be changed in the future again. We may want to forgo this PR or adjust it if this change is landing soon-ish, e.g. documentation exists and/ or code exists in some branch. If I know what to change, I can adjust the PR. On a second thought: I think flake-compat is already more lenient than real flakes. For example, I had to change some code that was building temporary paths to /etc, which obviously isn't allowed in a real flake (but flake-compat cannot check for this). We might be ok with flake-compat being simple and a superset of (rather than identical to) real flakes, right?

jorsn commented

2021-02-26 23:21:28 +00:00

(Migrated from github.com)

Well, currently flake-compat is more strict than real flakes by not allowing relative paths. I don't think it is clear wether "path:../" will be allowed in the future, but relative paths in the top-level flake will probably stay possible if you look at the discussions in https://github.com/NixOS/nix/issues/3978 and https://github.com/NixOS/nix/issues/4218. The RFC also lists the path input type without mentioning wether it's relative or absolute.

Well, currently flake-compat is more strict than real flakes by not allowing relative paths. I don't think it is clear wether "path:../" will be allowed in the future, but relative paths in the top-level flake will probably stay possible if you look at the discussions in https://github.com/NixOS/nix/issues/3978 and https://github.com/NixOS/nix/issues/4218. The [RFC](https://github.com/tweag/rfcs/blob/flakes/rfcs/0049-flakes.md#flake-inputs) also lists the `path` input type without mentioning wether it's relative or absolute.

👍 1

zimbatm commented

2023-04-03 14:20:42 +00:00

(Migrated from github.com)

added to https://github.com/nix-community/flake-compat