Conversation
nixpkgs-ci
bot
added
10.rebuild-linux: 1-10
tfc
force-pushed
the
unwhack-nrm
branch
3 times, most recently
from
4acdde7 to
67747fd
Compare
|
kmein
left a comment
kmein
left a comment
There was a problem hiding this comment.
Functionality-wise, everything looks really good. I've left a few inline comments throughout the patch—mostly just some naming questions and minor nitpicks around type hints and a small optimization.
Let me know what you think of those, but overall this is in great shape!
| # we need to resolve paths before concatenation because of things like | ||
| # $ ls -l /sys/dev/char/226:128/subsystem | ||
| # ... /sys/dev/char/226:128/subsystem | ||
| # -> ../../../../../../class/drm | ||
| # Path(normpath(...)) to normalize `foo/../bar` to `bar` | ||
| p = Path(os.path.normpath(p.parent.resolve() / parent)) |
There was a problem hiding this comment.
Reflecting whether p.parent.resolve() might lead to missing paths the way p.resolve() does. Maybe not because we directly mount p?
|
TODO:
|
pkgs/by-name/ni/nix-required-mounts/nix-required-mounts/test_nix_required_mounts.py
Outdated
Show resolved
Hide resolved
tfc
force-pushed
the
unwhack-nrm
branch
2 times, most recently
from
d6e144a to
f2797ce
Compare
ConnorBaker
left a comment
ConnorBaker
left a comment
There was a problem hiding this comment.
Thank you for making tests! I only had the chance to read through part of it (apologies) and left a few comments.
| # see also test_path_dsicovery_resolve_rel_links | ||
| # | ||
| # Path(normpath(...)) needed to normalize `foo/../bar` to `bar` | ||
| p = Path(os.path.normpath(p.parent.resolve() / parent)) |
There was a problem hiding this comment.
Why is it not sufficient to always just do p.absolute()? I understand there are some nuances around normalization: https://docs.python.org/3/library/pathlib.html#comparison-to-the-os-and-os-path-modules
There was a problem hiding this comment.
.absolute() does not resolve the .. part.
| def symlink_paths_closure(p: Path) -> List[Path]: | ||
| """Traverses a chain of symlinks to collect every intermediate path up to the final destination.""" |
There was a problem hiding this comment.
To clarify, do this function collect both the location of each symlink as well as its target?
That is, if we have
- /
- foo
- b -> /bar
- bar
- c -> /baz
- baz
- a -> /foo
what would we expect /a/b/c to return?
There was a problem hiding this comment.
That is a very good additional test case that unearths something that we don't handle at all right now.
currently we would simply return ["/baz"].
I do realize that we should return ["/foo", "/bar", "/baz"].
As this PR fixes a lot of problems that are different to this one, i suggest building that as a followup. What do you think?
There was a problem hiding this comment.
There was a problem hiding this comment.
That's not the same scenario.
(@SomeoneSerge may i please ask you politely to write more detailed responses in your comments in this PR? i often struggle to understand what you actually mean in some of your comments)
There was a problem hiding this comment.
That's not the same scenario.
Right, misreading on my part
There was a problem hiding this comment.
I addressed this now, including test cases.
jfly
left a comment
jfly
left a comment
There was a problem hiding this comment.
Very incomplete review, sorry. Posting what I did get through.
pkgs/by-name/ni/nix-required-mounts/nix-required-mounts/src/nix_required_mounts.py
Outdated
Show resolved
Hide resolved
pkgs/by-name/ni/nix-required-mounts/nix-required-mounts/src/nix_required_mounts.py
Outdated
Show resolved
Hide resolved
pkgs/by-name/ni/nix-required-mounts/nix-required-mounts/src/nix_required_mounts.py
Outdated
Show resolved
Hide resolved
pkgs/by-name/ni/nix-required-mounts/nix-required-mounts/src/nix_required_mounts.py
Outdated
Show resolved
Hide resolved
| pyproject = true; | ||
|
|
||
| src = lib.cleanSource ./.; | ||
| src = lib.cleanSource ./nix-required-mounts; |
There was a problem hiding this comment.
I don't think there's much point in moving source into a subdirectory
There was a problem hiding this comment.
your suggestion to provide a pytest hook transitively made it necessary to give this project a real python package structure. if you check the code out and can make it work without this structure, i am happy to accept your patch.
|
It works in the other PR:)
…On 20 March 2026 13:04:30 UTC, Jacek Galowicz ***@***.***> wrote:
@tfc commented on this pull request.
> @@ -37,13 +37,20 @@ python3Packages.buildPythonApplication {
inherit pname version;
pyproject = true;
- src = lib.cleanSource ./.;
+ src = lib.cleanSource ./nix-required-mounts;
your suggestion to provide a pytest hook transitively made it necessary to give this project a real python package structure. if you check the code out and can make it work without this structure, i am happy to accept your patch.
|
@SomeoneSerge when i run this in your PR branch: Executing pytestCheckPhase
pytest flags: -m pytest
============================= test session starts ==============================
platform darwin -- Python 3.13.12, pytest-9.0.2, pluggy-1.6.0
rootdir: /nix/var/nix/builds/nix-13868-663597863/source
configfile: pyproject.toml
collected 1 item
nix_required_mounts.py . [100%]
============================== 1 passed in 0.01s ===============================
Finished executing pytestCheckPhaseit works but does not contain tests. as soon as i throw a test_bla.py python into the folder, pytest comes up with all kinds of errors that led me to create a "real project" folder. |
tfc
force-pushed
the
unwhack-nrm
branch
4 times, most recently
from
500dbbc to
3064232
Compare
|
@tfc Could you allow maintainer edits please? EDIT: Ah, nevermind. Need a rebase prior to push |
...for guest paths handling
|
@tfc I moved the tests into the docstrings so they're easier to read (to me), renamed I expect that we squash prior to merging, and we can abstain from force-pushing until then as to avoid more conflicts. EDIT: I am open to simply prohibiting some non-trivial cases of |
| >>> print(pformat(paths_chain_jump)) | ||
| [('${TMP}/chain/a', '${TMP}/chain/a'), | ||
| ('/guest/chain/b', '${TMP}/chain/b'), | ||
| ('/guest/chain/c', '${TMP}/chain/c'), | ||
| ('/guest/jump/a', '${TMP}/jump/a'), | ||
| ('/guest/jump/c/d', '${TMP}/jump/c/d')] |
There was a problem hiding this comment.
337 >>> print(pformat(paths_chain_jump))
Differences (ndiff with -expected +actual):
[('${TMP}/chain/a', '${TMP}/chain/a'),
- ('/guest/chain/b', '${TMP}/chain/b'),
? ^^^^^^
+ ('${TMP}/chain/b', '${TMP}/chain/b'),
? ^^^^^^
+ ('${TMP}/jump/c/d', '${TMP}/jump/c/d'),
('/guest/chain/c', '${TMP}/chain/c'),
- ('/guest/jump/a', '${TMP}/jump/a'),
? ^
+ ('/guest/jump/a', '${TMP}/jump/a')]
? ^
- ('/guest/jump/c/d', '${TMP}/jump/| ... results_empty = subst_results(match_mounts(allowed_patterns, [])) | ||
| ... results_a = subst_results(match_mounts(allowed_patterns, ["feat_a"])) | ||
| ... results_b = subst_results(match_mounts(allowed_patterns, ["feat_b2"])) |
There was a problem hiding this comment.
- The latter two tests are really about what's currently called
validate_mountsrather than aboutmatch_mounts match_mounts's tests, otoh, should cover something like "we request 2 features out of 3"
|
@SomeoneSerge thanks, i hate it... i don't really see why we have to cram eeeeeverything into one file, it looks like quite a mess to be honest. Apart from the test changes, can you please explain what you are doing there on top of the doctest related changes? i see that there is some host-guest-diff work, but right now it feels like my work is taken over from me and i don't understand your thinking so it makes it difficult to participate in the discussion on how to bring this PR further. |
I am validating your tests (at least one I suspect to be wrong, cf. the failure) and trying to make the new names match the actual functionality.
We don't, it just so happened that once I moved what seemed like simple-enough examples into the docstrings there wasn't anything left in the other file. I do, however, need tests to be localized so that I can compare the environment definition against the "expected output", which is how I came to question the Feel free to move the more bulky test back into DetailsP.S. Perhaps https://www.youtube.com/watch?v=XpDsk374LDE might help you relax |
|
@SomeoneSerge Thanks for the feedback. I’m happy to move the tests back, but I’d like to align on our workflow to save us both some time. It feels like we’ve reached a point where it felt faster/easier for you to rewrite the implementation than to communicate the specific requirements upfront. This makes it difficult for me to verify the new logic against the original bugs, effectively doubling the work for both of us. Moving forward, if you have specific preferences for structure, please let me know—I’m more than willing to adapt the code to meet community standards once the technical goal is clear. Proposed Changes:
Let me know what you think. I'll proceed once we're on the same page so we can reach a mergeable state quickly without further rework. |
|
- "point where it felt easier to rewrite": no worries, I won't do it twice; but yes, it was easier to show, than to explain e.g. the setuptools idiosyncrasies.
- "revert tests to ...": whether you revert and re-apply, or iterate from the current state, please let's have the smallest tests in the docstrings, and the larger in the external file
- "test inspectability": validating the environment is just an extra assert, and even if you wanted interactive debugging, which you don't, it's one `yield` away, but honestly idc about any or that, I care about being able to tell if the test tests the right thing
- "clarity&formatting": this is just ricing
- "naming": the function absolutely _is_ mounts-specific, it's translating wildcards and symilnks on the host into guest paths in a very niche manner, it's walking the tree in a niche and a priori surprising fashion; at the same time it's not taking care of very many other ways in which the mounted paths might end up invalid: references other than symlinks (including store path references), submounts, ...; the docstring should say something like "complete/expand the list of mounts/patterns in such a way that the symlinks resolve correctly in the guest".
- whichever way you choose, please keep further commits additive for now so I'm able to pull without resetting, ty
Cheers
…On 21 March 2026 13:05:33 UTC, Jacek Galowicz ***@***.***> wrote:
tfc left a comment (NixOS/nixpkgs#500971)
@SomeoneSerge Thanks for the feedback. I’m happy to move the tests back, but I’d like to align on our workflow to save us both some time.
It feels like we’ve reached a point where it felt faster/easier for you to rewrite the implementation than to communicate the specific requirements upfront. This makes it difficult for me to verify the new logic against the original bugs, effectively doubling the work for both of us. Moving forward, if you have specific preferences for structure, please let me know—I’m more than willing to adapt the code to meet community standards once the technical goal is clear.
Proposed Changes:
- Revert tests to `test_....py`:
- Verifiability: Removing the one central `tidyUp` function of the test file allows for quick and easy terminal inspection of the correctness of all symlink scenarios after a `pytest` run, which is currently made more difficult by the fine grained use of the guard pattern (i'm all in towards using as many tight guards as possible in production code, but in the tests they make everything less inspectable).
- Code Clarity: Moving tests out of doctests reduces noise in the main application logic. Currently, the new added boilerplate makes it difficult for newcomers to distinguish between code that only exists for testing and the code that the app needs in production. We had less boilerplate in the external test case.
- Tooling: The current doctests aren't subject to black formatting, which impacts long-term maintainability.
- Bug Fix: I will incorporate and fix the specific test case you identified.
- Naming: I suggest reverting the function rename. The logic generically finds path closures by following symlinks; naming it after "mounts" (the current use case) is misleading, as the function itself is agnostic of mount-specific details.
Let me know what you think. I'll proceed once we're on the same page so we can reach a mergeable state quickly without further rework.
|
5 participants
I have been running cuda apps on both NVIDIA and AMD (with ZLUDA) graphics cards and found that nix-required-mounts produced some problems:
While fixing these issues, i decided that the scenario is complex enough for test cases, so i wrote those and restructured the app a bit to make it more testable.
Tested these changes again on two NVIDIA and AMD machines, works very nicely.
Fixes #497824
CC @SomeoneSerge @ConnorBaker @kmein @jfly
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.