lib.path.subpath.join: init #209099

infinisil · 2023-01-04T22:23:57Z

Description of changes

This function can be used to safely join subpaths together, a bit like concatStringsSep "/", but safer. Relates to other work in the path library effort.

This work is sponsored by Antithesis ✨

Things done

Wrote documentation
Added and ran tests

lib/path/default.nix

lib/path/tests/prop.nix

fricklerhandwerk · 2023-01-13T10:06:43Z

lib/path/tests/prop.nix

+  # Check the properties for all lists of strings
+  subpathJoinCheckAll =
+    assert all subpathJoinCheck lists;
+    null;
+
+in builtins.seq subpathJoinCheckAll
+  (genAttrs strings normaliseAndCheck)


This is a bit ugly. Given that we will extend the test suite anyway (I haven't looked at the other PRs yet), maybe we should do a little bit of generalisation. I feel uneasy about the change to the randomly generated paths, because it makes the whole setup more complicated and significantly harder to understand before getting into the actual testing. Have you considered approaching this differently? Instead of producing lists of paths in the random generator, produce a sequence of random numbers (which can be reused for other purposes), pass that and do the chunking in Nix.

Maybe we could also have one Nix file per test, and import each from an entry point that does the pre-processing of the input? Then each file can take whatever data type it cares about, directly. The main file will then only combine the tests in a way that produces output as needed for further processing back in the shell. I suppose, with the other tests, we will have a series of asserts and then produce a value, but having that in one place without the other noise will allow for easier changes of factoring.

It would be nice not to have the generation logic in Nix expressions, to decouple the Nix tests from the test runner. However, we also use this pattern for invoking external commands that we model in Nix, such as realpath, so we can't easily get away from all coupling between framework and tests.

I do think it's worth exploring for the sake of writing Nix-only tests in a consistent style where all domain-specific testing logic is in the same language.
Instead of generating examples or a large chunk of randomness ahead of time, we could easily create a pseudorandom number generator by invoking builtins.hashString "sha256" repeatedly on a state variable, starting from a small seed that's generated outside of Nix. This achieves the goal without ever running out of randomness, and therefore without any coupling with the non-Nix world. The state variable can be handled by a simplified Nix equivalent of the quickcheck Gen library. Instead of applicative we could have a single function that takes an attrset of "gens" and a function matching those attr names, etc.

I don't agree we should push toward implementing more things in the Nix language, even if it's possible and not all that hard to do. It will happen anyway, but should happen much more slowly and deliberately. That is, I wouldn't oppose a QuickCheck implementation or PRs to migrate existing tests to it. But for the sake of testing this library, simple Nix expressions that batch-process inputs should do.

I'm just concerned with adding too many things at once and losing track of what we want to achieve in a given PR.

Since this is test code, we could maybe even allow using the cursed builtins.exec:

$ nix-instantiate --eval -E 'builtins.exec [ "realpath" "-m" "//foo/./bar/." ]' --allow-unsafe-native-code-during-evaluation /foo/bar

But no I don't think it would be a great idea to write all tests in Nix itself. Nix is just not made for this kind of thing. One of the main restrictions is that we can't check for all kinds of evaluation failures. builtins.tryEval only works for throw and assert, no other failures, and it doesn't allow checking for the error message either. Randomness too is not something Nix was made for. Yes we can hack around it with builtins.hashString, custom PRNG's (I even wrote one in Nix myself some time ago) and monadic generator passing, but that's gonna be painful.

I think something like https://github.com/Mic92/pythonix is the right way to go. Unfortunately that project is archived now, but it works by linking to libexpr directly and using that to evaluate expressions. This way failures can be caught and checked. The main problem with this is that libexpr isn't stable, so it requires continuous maintenance. But using that approach testing libraries of generic programming languages can be used.

The alternative of just using the CLI as a stable API has the problem that it's way too slow (this is why I opted for just a single CLI call in these tests here).

I'll see if I can do some more decoupling for the property tests here though. Maybe having a separate file/folder for every property to check and using the .nix file only for batch evaluation and no checking.

I started this: #212858

I think it would be better to just drop the property tests from this PR. I don't want to have this be blocked on improved property tests. There's still unit tests as well.

fricklerhandwerk · 2023-01-13T10:08:15Z

lib/path/tests/generate.awk

+  uppernewstring = upperslash + 1 + extranullweight
+
+  total = uppernewstring + 1 + extraelemweight


I'm really not fond of this extra complexity, see the comment at the end of the Nix file.

lib/path/default.nix

- Use isValid when possible instead of subpathInvalidReason: NixOS#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS#208887 (comment) - Add short comments for the unit test groups: NixOS#208887 (comment)

- Use isValid when possible instead of subpathInvalidReason: NixOS#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS#208887 (comment) - Add short comments for the unit test groups: NixOS#208887 (comment) - Slight formatting improvement for laws: NixOS#209099 (comment)

infinisil · 2023-02-07T17:06:14Z

@fricklerhandwerk @roberth I'm now working on NixOS/nix#7735 to have better Nix testing support in the future. For now I removed the more hacky property tests from this PR so it's not blocked on that, there's still unit tests in any case.

I also rebased this PR after the merge of #208887, to me this PR looks good like this

infinisil · 2023-02-07T17:45:35Z

lib/path/default.nix

+    assert assertAll isValid subpaths (i: path: ''
+      lib.path.subpath.join: Element at index ${toString i} is not a valid subpath string:
+          ${subpathInvalidReason path}'');


I simplified this assertion a lot by introducing lib.asserts.assertAll, now included in this PR. #209375 will also be able to make use of that.

Never mind, I split that function into #215166

infinisil · 2023-02-07T18:08:36Z

lib/path/default.nix

+    assert assertMsg
+      # Fast in case all paths are valid
+      (all isValid subpaths)
+      # Otherwise we take our time to gather more info for a better error message
+      # Strictly go through each path, throwing on the first invalid one
+      # Tracks the list index in the fold accumulator
+      (foldl' (i: path:
+        if isValid path
+        then i + 1
+        else throw ''
+          lib.path.subpath.join: Element at index ${toString i} is not a valid subpath string:
+              ${subpathInvalidReason path}''
+      ) 0 subpaths);


I also simplified this a bit to remove the seq and abort fallthru thing, I think that was overengineered.

I really don't like the abuse of the exception "side effect" here.
It starts with assert, which is never triggered and then it continues with assertMsg, which is also never triggered. The final error happens to be in the error message of assertMsg, evaluated when it almost triggers.
This is enough reason to just use an if expression instead.

Final nitpick: assertions are for checking the invariants of self-contained software units, and not for input validation across a public interface. See assert (verb)

1. To declare with assurance or plainly and strongly; to state positively.
[...]
(programming) To specify that a condition or expression is true at a certain point in the code.

Good point, using an if now.

Regarding assertions in general, yeah maybe assert is being abused in that way in Nix, just because it's somewhat easier to type. For now it seems to be the pattern though, so I still think that with #215166 it's okay to replace this if here with an assert assertAll isValid subpaths

- Use isValid when possible instead of subpathInvalidReason: NixOS/nixpkgs#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS/nixpkgs#208887 (comment) - Add short comments for the unit test groups: NixOS/nixpkgs#208887 (comment) - Slight formatting improvement for laws: NixOS/nixpkgs#209099 (comment)

- Use isValid when possible instead of subpathInvalidReason: NixOS#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS#208887 (comment) - Add short comments for the unit test groups: NixOS#208887 (comment) - Slight formatting improvement for laws: NixOS#209099 (comment)

- Use isValid when possible instead of subpathInvalidReason: NixOS/nixpkgs#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS/nixpkgs#208887 (comment) - Add short comments for the unit test groups: NixOS/nixpkgs#208887 (comment) - Slight formatting improvement for laws: NixOS/nixpkgs#209099 (comment)

This function can be used to safely join subpaths together

- Use isValid when possible instead of subpathInvalidReason: NixOS#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS#208887 (comment) - Add short comments for the unit test groups: NixOS#208887 (comment) - Slight formatting improvement for laws: NixOS#209099 (comment)

infinisil · 2023-03-14T18:34:18Z

This PR isn't strictly blocked by anything, so I'm going to merge this soon

- Use isValid when possible instead of subpathInvalidReason: NixOS/nixpkgs#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS/nixpkgs#208887 (comment) - Add short comments for the unit test groups: NixOS/nixpkgs#208887 (comment) - Slight formatting improvement for laws: NixOS/nixpkgs#209099 (comment)

infinisil requested review from roberth, edolstra and fricklerhandwerk January 4, 2023 22:23

infinisil requested a review from nbp as a code owner January 4, 2023 22:23

This was referenced Jan 4, 2023

lib.path.append: init #208887

Merged

Path library design #200718

Closed

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Jan 4, 2023

This was referenced Jan 6, 2023

lib.path.difference: init #209375

Closed

Path library functions #210426

Closed

fricklerhandwerk reviewed Jan 13, 2023

View reviewed changes

lib/path/default.nix Outdated Show resolved Hide resolved

infinisil force-pushed the lib.path.subpath.join branch 2 times, most recently from 620a295 to 02b79c8 Compare January 18, 2023 16:54

infinisil force-pushed the lib.path.subpath.join branch from 02b79c8 to bbc4bfc Compare January 18, 2023 17:21

This was referenced Jan 26, 2023

lib: Create a generic property testing framework #212858

Closed

Experimental python bindings NixOS/nix#7735

Draft

infinisil force-pushed the lib.path.subpath.join branch from bbc4bfc to 325009d Compare February 7, 2023 16:31

infinisil requested a review from Profpatsch as a code owner February 7, 2023 17:43

infinisil changed the title ~~lib.path.subpath.join: init~~ lib.path.subpath.join, lib.asserts.assertAll: init Feb 7, 2023

infinisil commented Feb 7, 2023

View reviewed changes

infinisil mentioned this pull request Feb 7, 2023

lib.asserts.assertAll: init #215166

Open

3 tasks

infinisil force-pushed the lib.path.subpath.join branch from 4930120 to 1fc4e26 Compare February 7, 2023 18:06

infinisil changed the title ~~lib.path.subpath.join, lib.asserts.assertAll: init~~ lib.path.subpath.join: init Feb 7, 2023

infinisil commented Feb 7, 2023

View reviewed changes

lib.path.subpath.join: init

1a2c284

This function can be used to safely join subpaths together

infinisil force-pushed the lib.path.subpath.join branch from 1fc4e26 to 1a2c284 Compare February 13, 2023 13:02

infinisil mentioned this pull request Feb 15, 2023

[RFC 0135] Custom asserts NixOS/rfcs#135

Closed

roberth approved these changes Mar 14, 2023

View reviewed changes

infinisil merged commit b611afe into NixOS:master Mar 14, 2023

infinisil deleted the lib.path.subpath.join branch March 14, 2023 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib.path.subpath.join: init #209099

lib.path.subpath.join: init #209099

infinisil commented Jan 4, 2023 •

edited

fricklerhandwerk Jan 13, 2023

roberth Jan 14, 2023

fricklerhandwerk Jan 16, 2023 •

edited

infinisil Jan 17, 2023

infinisil Jan 26, 2023

infinisil Jan 27, 2023

fricklerhandwerk Jan 13, 2023

infinisil commented Feb 7, 2023

infinisil Feb 7, 2023 •

edited

infinisil Feb 7, 2023

infinisil Feb 7, 2023

roberth Feb 13, 2023

infinisil Feb 13, 2023

infinisil commented Mar 14, 2023

		uppernewstring = upperslash + 1 + extranullweight

		total = uppernewstring + 1 + extraelemweight

lib.path.subpath.join: init #209099

lib.path.subpath.join: init #209099

Conversation

infinisil commented Jan 4, 2023 • edited

Description of changes

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fricklerhandwerk Jan 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Feb 7, 2023

infinisil Feb 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Mar 14, 2023

infinisil commented Jan 4, 2023 •

edited

fricklerhandwerk Jan 16, 2023 •

edited

infinisil Feb 7, 2023 •

edited