Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib.fileset.trace, lib.fileset.traceVal: init #256417

Merged
merged 9 commits into from
Oct 4, 2023

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Sep 20, 2023

Description of changes

Another split-off from the main file set combinators PR. This one adds two functions for debugging file sets:

  • lib.fileset.trace: Trace a file set and return a value
    trace (unions [ ./Makefile ./src ./tests/run.sh ]) null
  • lib.fileset.traceVal: Same, but it returns the file set itself
    toSource {
      root = ./.;
      fileset = traceVal (unions [
        ./Makefile
        ./src
        ./tests/run.sh
      ]);
    }

These both print the file set like this:

trace: /home/user/src/myProject
trace: - Makefile (regular)
trace: - src (all files in directory)
trace: - tests
trace:   - run.sh (regular)

This work is sponsored by Antithesis

Things done

  • Implementation
    • Tested
    • Commented
  • Added user documentation

@github-actions github-actions bot added 8.has: documentation 6.topic: lib The Nixpkgs function library labels Sep 20, 2023
@infinisil infinisil mentioned this pull request Sep 20, 2023
7 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Sep 21, 2023
@infinisil infinisil marked this pull request as ready for review September 21, 2023 11:33
lib/fileset/internal.nix Outdated Show resolved Hide resolved
@infinisil infinisil force-pushed the fileset.trace branch 2 times, most recently from 08c75df to 0fc1619 Compare September 27, 2023 19:53
@infinisil
Copy link
Member Author

Removed the seq foldl' hack now that #256544 is merged.

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The weird thing is that the stack overflow avoidance tests fail on my NixOS 23.05 with Nix 2.13.3.

test case at lib/fileset/tests.sh:523 failed: toSource { root = ./.; fileset = unions (
mapAttrsToList (name: _: ./. + "/${name}/a") (builtins.readDir ./.)); } failed to evalu
ate, but it was expected to succeed
lib/fileset/tests.sh: line 124: 18590 Segmentation fault      (core dumped) nix-instant
iate --eval --show-trace --expr "$prefixExpression (trace $config $expr)" > /dev/null 2
> "$tmp"/stderrTrace
lib/fileset/tests.sh: line 124: 18596 Segmentation fault      (core dumped) nix-instant
iate --eval --show-trace --expr "$prefixExpression (trace $config $expr)" > /dev/null 2
> "$tmp"/stderrTraceVal
test case at lib/fileset/tests.sh:635 failed: ./. should have traced this:

/run/user/1000/tmp.Ue0Y2xun3k/work (all files in directory)

but this was actually traced:


lib/fileset/tests.sh Show resolved Hide resolved
lib/fileset/tests.sh Outdated Show resolved Hide resolved
lib/fileset/tests.sh Outdated Show resolved Hide resolved
lib/fileset/default.nix Show resolved Hide resolved
lib/fileset/internal.nix Outdated Show resolved Hide resolved
lib/fileset/internal.nix Outdated Show resolved Hide resolved
lib/fileset/internal.nix Show resolved Hide resolved
Comment on lines 633 to 634
# Locally limit the stack to 100 * 1024 bytes, this would cause a stack overflow if the short-circuiting isn't implemented
ulimit -s 100
Copy link
Member Author

@infinisil infinisil Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fricklerhandwerk (using this thread to reply to #256417 (review))

The weird thing is that the stack overflow avoidance tests fail on my NixOS 23.05 with Nix 2.13.3.

Interesting, does it happen both with nix-build lib/tests/release.nix and lib/fileset/tests.sh?

I tried reproducing this with

diff --git a/lib/tests/release.nix b/lib/tests/release.nix
index c8d6b810122e..02067a79f9a6 100644
--- a/lib/tests/release.nix
+++ b/lib/tests/release.nix
@@ -2,7 +2,12 @@
   # Don't test properties of pkgs.lib, but rather the lib in the parent directory
   pkgs ? import ../.. {} // { lib = throw "pkgs.lib accessed, but the lib tests should use nixpkgs' lib path directly!"; },
   nix ? pkgs.nix,
-  nixVersions ? [ pkgs.nixVersions.minimum nix pkgs.nixVersions.unstable ],
+  nixVersions ? [
+    pkgs.nixVersions.minimum
+    pkgs.nixVersions.nix_2_13
+    nix
+    pkgs.nixVersions.unstable
+  ],
 }:
 
 let

and nix-build lib/tests/release.nix -A paths.1, but no luck. I also tried downgrading from the current 2.13.5 to 2.13.3 by reverting bcb02bd, but that also didn't cause it to break.

I can kind of see how it could happen though: It looks like Nix segfaults if there's too little stack space, though for me that only happens for <44*1024 bytes:

$ ulimit -s 44
$ nix-env --version
nix-env (Nix) 2.15.1
$ ulimit -s 43
$ nix-env --version
zsh: segmentation fault (core dumped)  nix-env --version

Could you test whether you can run the tests on master? Because there's already another test doing this too:

# Locally limit the maximum stack size to 100 * 1024 bytes
# If unions was implemented recursively, this would stack overflow
ulimit -s 100

@roberth Testing the stack like this might be machine dependent I guess, might be better to drop it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infinisil it doesn't fail with or without ulimit when using nix-build, but does the same as previously when running test script directly. Note though that the output store path when using nix-build is /nix/store/7gf7wnp28h77gl5y1k2pw3n2pmxg6b57-nixpkgs-lib-tests-nix-2.17.0.

@infinisil
Copy link
Member Author

I was able to not make the new test here rely on ulimit -s, I also created #258080 to remove the other use of ulimit -s.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 2, 2023
lib/fileset/default.nix Show resolved Hide resolved
lib/fileset/tests.sh Show resolved Hide resolved
lib/fileset/default.nix Outdated Show resolved Hide resolved
@roberth
Copy link
Member

roberth commented Oct 3, 2023

Probably needs adaptation for

@infinisil
Copy link
Member Author

infinisil commented Oct 3, 2023

Rebased to fix conflicts and removed the {} argument from trace/traceVal as suggested. This even makes this more consistent with builtins.trace and lib.debug.traceVal, as those also can't be customised.

Here's the easily-viewable diff of the changes: https://github.com/tweag/nixpkgs/compare/55c1b0b462d28d404f78695e7d8d8885f81fe080..53c053be347989bff8c08769f08561ced200b21f

@roberth
Copy link
Member

roberth commented Oct 4, 2023

Thanks for the diff link! That helps a lot.

I don't see a test case for trace (unions [ ]). Could you add it?

@infinisil
Copy link
Member Author

@roberth Oh good catch, I didn't think of the interaction after #257351, it indeed didn't work at all and needed to be fixed! That's done now, diff here (this one GitHub also links to)

@infinisil infinisil merged commit 5db719f into NixOS:master Oct 4, 2023
6 checks passed
@infinisil infinisil deleted the fileset.trace branch October 4, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants