Remove All Absolute Path Literals From Nixpkgs

Development
1

While reading the release notes of Nix v2.34, I learned about the lint-absolute-path-literals option (docs). I also learned about absolute path literals and how they are not portable.

So I set lint-absolute-path-literals to warn in my config. And now I got a bunch of warnings about absolute path literals being used in Nixpkgs.

So I set out to remove all absolute path literals from Nixpkgs. I made the following PRs so far:

I don’t think I can fix the remaining cases on my own. I need advice from someone more experienced.

Case 1: lib/path/default.nix

This is the relevant code from lib/path/default.nix:

  hasStorePathPrefix =
    path:
    let
      deconstructed = deconstructPath path;
    in
    assert assertMsg (isPath path)
      "lib.path.hasStorePathPrefix: Argument is of type ${typeOf path}, but a path was expected";
    assert assertMsg
      # This function likely breaks or needs adjustment if used with other filesystem roots, if they ever get implemented.
      # Let's try to error nicely in such a case, though it's unclear how an implementation would work even and whether this could be detected.
      # See also https://github.com/NixOS/nix/pull/6530#discussion_r1422843117
      (deconstructed.root == /. && toString deconstructed.root == "/")
      "lib.path.hasStorePathPrefix: Argument has a filesystem root (${toString deconstructed.root}) that's not /, which is currently not supported.";
    componentsHaveStorePathPrefix deconstructed.components;

To me, deconstructed.root == /. and toString deconstructed.root == "/" are the same condition repeated twice. But of course I’m wrong and there is a reason for this that invalidates my whole intuition about how paths and strings are related.

Case 2: lib/filesystem.nix

This is the relevant code from lib/filesystem.nix:

  locateDominatingFile =
    pattern: file:
    let
      go =
        path:
        let
          files = builtins.attrNames (builtins.readDir path);
          matches = builtins.filter (match: match != null) (map (builtins.match pattern) files);
        in
        if builtins.length matches != 0 then
          { inherit path matches; }
        else if path == /. then
          null
        else
          go (dirOf path);
      parent = dirOf file;
      isDir =
        let
          base = baseNameOf file;
          type = (builtins.readDir parent).${base} or null;
        in
        file == /. || type == "directory";
    in
    go (if isDir then file else parent);

Here we can:

  • Replace path == /. with toString path == "/"
  • Replace file == /. with toString file == "/"

Looks straightforward. I just need confirmation.

Case 3: lib/sources.nix

This is the relevant code from lib/sources.nix:

  absolutePath =
    base: path: if lib.hasPrefix "/" path then path else toString (/. + "${base}/${path}");

I’m assuming the conversion from string to path then again to a string is to guard against base not starting with /. If my assumption is correct we can remove the absolute path literal by:

  absolutePath =
    base: path:
    if lib.hasPrefix "/" path then
      path
    else if lib.hasPrefix "/" base then
      "${base}/${path}"
    else
      "/${base}/${path}";

Case 4: lib/types.nix

This is the relevant code from lib/types.nix:

  isInStore = lib.path.hasStorePathPrefix (
    if builtins.isPath x then
      x
    # Discarding string context is necessary to convert the value to
    # a path and safe as the result is never used in any derivation.
    else
      /. + builtins.unsafeDiscardStringContext x
  );

I have absolutely no idea on this one.

Sorry for the long post. I would appreciate any help with this.

6 Likes
2

Warnings are warnings and not errors because there are always false positives.

I don’t see any problems with the functions you show. When manipulating paths in the Nix language, you are dealing with path values which are always absolute paths. If you (re)construct them, you’ll need to use /. which is perfectly portable (except for potential windows support, but the code you show assumes POSIX anyways).

Furthermore, the changes you propose often unnecessarily complicate the code in question for no benefit.

As an example, /. + str is practically the only way to convert a string to a path value in Nix.

3

Thanks @sternenseemann for your response.

Can you please elaborate a bit more on /. being perfectly portable? It it a special case? Is it the only such special case?

It’s true that warnings are warnings, but the feature is unusable if I will keep getting a lot of warnings each time I evaluate something that involves Nixpkgs.

The options I see are:

  1. Remove all absolute path literals from Nixpkgs.
  2. Add exceptions to portable special cases to Nix itself.
  3. Don’t use lint-absolute-path-literals at all.

I can already see you don’t agree with the first option. Do you think the second one is a good idea?

2 Likes
4

Hrm… what I’d try to do is ping the author of this new lint and ask them what they think and whether they could help explain these cases that you don’t. Sorry, I can’t currently get into this more.

5

Thanks for the idea @mightyiam. Totally makes sense.

After a quick search:

Relevant Issue by @roberth:
NixOS/nix#8738 - no-absolute-path-literals experimental feature

Relevant PR by @Ericson2314:
NixOS/nix#15345 - Create lint for absolute path literals

6

@sternenseemann, I noticed something interesting in the issue:

Describe the solution you’d like

Add an experimental feature flag or setting that disables absolute path literals such as /. and /home or /var/lib/foo.

It specifically mentions /. as something that we don’t want.

7

@roberth and @Ericson2314, I would really appreciate your input here.

8

Thank you @furanchuchu for applying this lint to Nixpkgs!

I’ll start with my general observations, with case by case comments below.
1 seems to be the only actual false positive. The others can be fixed, though 4 shows that the linter’s focus on real files isn’t always in line with how a path value is used in practice. Using a path value as a “normalized list of path components” data type is convenient, even when the path does not refer to file system nodes that can be read in evaluation.
I would consider such a path an antipattern when it does not refer to file system nodes that can be read in evaluation, but alas.
If we want to enforce this lint in practice, I think we’ll want opt-outs, e.g. by means of a magic comment or something.

Case 1: This one is a false positive. It handles the possibility of roots that aren’t the global file system root. These don’t occur in any Nix implementation yet, but they would be an optimal solution to lazy fetching without hashing, so I’d like to keep this guard.

Case 2: your suggestion to check via toString looks ok to me.

Case 3: this is reconstructing a path value from some sort of string. Considering that this helper function seems to accept and return strings when the already start with "/", your suggestion looks ok to me.

Case 4: this uses a path value as an intermediate value to perform some surface-level normalization of the path syntax. This could be done independently using string operations in the else clause.
It is a portability problem though, but I’m not sure what the replacement should be. For now an opt-out would seem best, but for a true fix, I have to ask @Ericson2314 what he has in mind for this one.

7 Likes
9

Thanks for your detailed reply @roberth!

I created two PRs addressing case 2 and case 3 while were waiting for John’s response.

1 Like
10

@furanchuchu Thank you for looking into this! I was hoping that someone would investigate Nixpkgs after this was created.

Yes I agree with this. In part because of the way it is implemented in Nix, and in part because my own “directory file descriptor” mindset, I associate paths with wanting to actually open the file/dir, just as @roberth says. The other use-case is interesting.

I also wonder if a “parent path or null” built-in would be appropriate, because then we can do == /.-like stuff without creating a throw-way literal. I think it would also make locateDominatingFile more elegant.

Yeah that’s a tough one. I don’t know what I want to do for it either. I just know that long term, I would love it if we did not fix a choice of store dir at eval time and all, and that would side-step the issue.

1 Like
11

Thanks for your response @Ericson2314.

I don’t understand the “roots that are not the global system root” thing you and @roberth are talking about. I would appreciate it if you pointed me to anything I can read to educate myself about this matter. I can RTFM from there.

But at least I understand that there is nothing for me to do about case 1 and case 4 at the moment.

Which is a bummer because now I can’t set lint-absolute-path-literals to fatal. I already set lint-url-literals and lint-short-path-literals to fatal and the perfectionist in me wanted the same for lint-absolute-path-literals lol.

12

/. + path always felt like a hack to me, but builtins.toFile (builtins.baseNameOf file) (builtins.readFile file)) is even worse IMO.

The fact that there’s a builtins.toPath function, but it doesn’t do this is very weird.

1 Like
13

@boomshroom I assume you’re talking about NixOS/nixpkgs#511569’s modification to:
nixos/modules/services/monitoring/prometheus/exporters/blackbox.nix

If I understand correctly, builtins.toPath does the same thing as /. + path, but it’s deprecated [Source].

That’s why I went for:
builtins.toFile (builtins.baseNameOf file) (builtins.readFile file))

There is no deprecated functionality used in it. And more importantly, it shouldn’t break existing configs.

I went for the simplest solution, but we can avoid the ugly
builtins.toFile (builtins.baseNameOf file) (builtins.readFile file))
by doing the following:

  • If enableConfigCheck is set to false, we don’t care as coerceConfigFile won’t get called.
  • If enableConfigCheck is set to true, only accept a path for configFile. So the else branch in enableConfigCheck calls builtins.throw.

What do you think about this? If you think it’s good I can make a PR with this change.

14

No, __toPath returns a string, not a path :rofl: that’s why it’s deprecated.

nix-repl> __toPath /nix
"/nix"

nix-repl> /. + /nix
/nix

nix-repl> /. + "nix"
/nix
3 Likes
15

@waffle8946 this is wild lol

I even checked it in my terminal because I couldn’t believe it.

1 Like