edolstra / flake-compat

MIT License
251 stars 71 forks source link

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

Open BBBSnowball opened 3 years ago

BBBSnowball commented 3 years ago

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)
BBBSnowball commented 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

Thanks :)

jorsn commented 3 years ago

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 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 3 years ago

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 3 years ago

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.

zimbatm commented 1 year ago

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