Skip to content

Comments

tests.nixpkgs-check-by-name: Fix for symlinked tempdirs#254435

Merged
infinisil merged 2 commits intoNixOS:masterfrom
tweag:fix-by-name-check-on-darwin
Sep 12, 2023
Merged

tests.nixpkgs-check-by-name: Fix for symlinked tempdirs#254435
infinisil merged 2 commits intoNixOS:masterfrom
tweag:fix-by-name-check-on-darwin

Conversation

@infinisil
Copy link
Member

@infinisil infinisil commented Sep 10, 2023

Description of changes

Reported by @Atemu at NixCon :).

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Sep 10, 2023
@Atemu
Copy link
Member

Atemu commented Sep 11, 2023

For context, what this PR intends to fix is a bug discovered on Darwin where the generated attrs list is put into a location that is behind a symlink. Including that location in the NIX_PATH and then trying to access it results in an error like this:

error:
       … while calling anonymous lambda

         at «string»:9:1:

            8| # - is_derivation: The result of `lib.isDerivation <attr>`
            9| {
             | ^
           10|   attrsPath,

       error: access to canonical path '/private/var/folders/xp/9_ry6h9x6l9gh_g32qspz0_40000gp/T/.tmpFbcNO0' is forbidden in restricted mode

On Darwin, /tmp is a symlink to /private/tmp/ and the attrs file is put in /tmp which means this bug is triggered every time. It should be reproducible on other platforms aswell though.

This might also be a Nix bug.

@infinisil infinisil force-pushed the fix-by-name-check-on-darwin branch from ad578fa to d5a6a8b Compare September 11, 2023 13:45
@infinisil infinisil changed the title nixpkgs-check-by-name: WIP darwin fix nixpkgs-check-by-name: Fix for symlinked tempdirs Sep 11, 2023
@infinisil infinisil force-pushed the fix-by-name-check-on-darwin branch 2 times, most recently from 8658742 to db44976 Compare September 11, 2023 13:53
@infinisil
Copy link
Member Author

Thanks, I added the error message to the code and commit message. I also added a test to make sure we don't run into this again.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

With my limited rust knowledge the diff LGTM but at runtime I get:

Executing cargoCheckHook
++ cargo test -j 10 --release --target aarch64-apple-darwin --frozen -- --test-threads=10
   Compiling nixpkgs-check-by-name v0.1.0 (/private/tmp/nix-build-nixpkgs-check-by-name.drv-0/source)
    Finished release [optimized] target(s) in 0.79s
     Running unittests src/main.rs (target/aarch64-apple-darwin/release/deps/nixpkgs_check_by_name-c15b59f9dc020547)

running 3 tests
test tests::test_case_sensitive ... ok(B
error: creating symlink from '/private/tmp/nix-build-nixpkgs-check-by-name.drv-0/source/test-tmp/var/nix/gcroots/profiles' to '/private/tmp/nix-build-nixpkgs-check-by-name.drv-0/source/test-tmp/var/nix/profiles': File exists
test tests::tests_dir ... FAILED(B
test tests::test_symlinked_tmpdir ... ok(B

failures:

---- tests::tests_dir stdout ----
Error: Failed test case override-different-file

Caused by:
    Failed to run command "nix-instantiate" "--eval" "--json" "--strict" "--readonly-mode" "--restrict-eval" "--show-trace" "--expr" "# Takes a path to nixpkgs and a path to the json-encoded list of attributes to check.\n# Returns an attribute set containing information on each requested attribute.\n# If the attribute is missing from Nixpkgs it\'s also missing from the result.\n#\n# The returned information is an attribute set with:\n# - call_package_path: The <path> from `<attr> = callPackage <path> { ... }`,\n#   or null if it\'s not defined as with callPackage, or if the <path> is not a path\n# - is_derivation: The result of `lib.isDerivation <attr>`\n{\n  attrsPath,\n  nixpkgsPath,\n}:\nlet\n  attrs = builtins.fromJSON (builtins.readFile attrsPath);\n\n  # This overlay mocks callPackage to persist the path of the first argument\n  callPackageOverlay = self: super: {\n    callPackage = fn: args:\n      let\n        result = super.callPackage fn args;\n      in\n      if builtins.isAttrs result then\n        # If this was the last overlay to be applied, we could just only return the `_callPackagePath`,\n        # but that\'s not the case because stdenv has another overlays on top of user-provided ones.\n        # So to not break the stdenv build we need to return the mostly proper result here\n        result // {\n          _callPackagePath = fn;\n        }\n      else\n        # It\'s very rare that callPackage doesn\'t return an attribute set, but it can occur.\n        {\n          _callPackagePath = fn;\n        };\n  };\n\n  pkgs = import nixpkgsPath {\n    # Don\'t let the users home directory influence this result\n    config = { };\n    overlays = [ callPackageOverlay ];\n  };\n\n  attrInfo = attr: {\n    # These names are used by the deserializer on the Rust side\n    call_package_path =\n      if pkgs.${attr} ? _callPackagePath && builtins.isPath pkgs.${attr}._callPackagePath then\n        toString pkgs.${attr}._callPackagePath\n      else\n        null;\n    is_derivation = pkgs.lib.isDerivation pkgs.${attr};\n  };\n\n  attrInfos = builtins.listToAttrs (map (name: {\n    inherit name;\n    value = attrInfo name;\n  }) attrs);\n\nin\n# Filter out attributes not in Nixpkgs\nbuiltins.intersectAttrs pkgs attrInfos\n" "--arg" "attrsPath" "/private/tmp/nix-build-nixpkgs-check-by-name.drv-0/.tmpsmDyqP/actual/.tmpMJuBGU" "-I" "/private/tmp/nix-build-nixpkgs-check-by-name.drv-0/.tmpsmDyqP/actual/.tmpMJuBGU" "--arg" "nixpkgsPath" "/private/tmp/nix-build-nixpkgs-check-by-name.drv-0/source/tests/override-different-file" "-I" "/private/tmp/nix-build-nixpkgs-check-by-name.drv-0/source/tests/override-different-file" "-I" "tests/mock-nixpkgs.nix"


failures:
    tests::tests_dir

test result: FAILED(B. 2 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.37s

error: test failed, to rerun pass `--bin nixpkgs-check-by-name`
error: builder for '/nix/store/krq0s9qg8ll4rzy46ih1mfbqj0lg2p8s-nixpkgs-check-by-name.drv' failed with exit code 101;

I assume this doesn't repro on Linux?

@infinisil
Copy link
Member Author

infinisil commented Sep 11, 2023

Interesting, this looks a bit like NixOS/nix#2706, though I don't fully understand it yet.

@Atemu Is this reproducible? Edit: I can reproduce it on Linux too! Only happens in the nix-build though

@infinisil
Copy link
Member Author

@ofborg build tests.nixpkgs-check-by-name

infinisil and others added 2 commits September 12, 2023 01:07
On Darwin, /tmp is sometimes a symlink to /private/tmp, which couldn't
be handled before:

    error: access to canonical path '/private/var/folders/xp/9_ry6h9x6l9gh_g32qspz0_40000gp/T/.tmpFbcNO0' is forbidden in restricted mode

This both fixes that and adds a test to make sure it can't happen again
We seem to have enough tests to run into this now:

    error: creating symlink from '/private/tmp/nix-build-nixpkgs-check-by-name.drv-0/source/test-tmp/var/nix/gcroots/profiles' to '/private/tmp/nix-build-nixpkgs-check-by-name.drv-0/source/test-tmp/var/nix/profiles': File exists
@infinisil infinisil force-pushed the fix-by-name-check-on-darwin branch from db44976 to 9c9a7e0 Compare September 11, 2023 23:07
@infinisil
Copy link
Member Author

infinisil commented Sep 11, 2023

Interesting, this looks a bit like NixOS/nix#2706, though I don't fully understand it yet.

Indeed this was it. Apparently cargo runs tests in parallel by default, which triggers this, combined with the fact that the Nix store in the nix-build was not initialised beforehand.

One way to fix this would be to make cargo run tests sequentially, which could be done with dontUseCargoParallelTests = true;, but that's a bit hacky.

Instead I'm going to copy the fix for this from https://github.com/tweag/lorri/pull/58: Initialising the store beforehand.

Btw I'm not sure why this wasn't a problem before when we already had two separate tests, we have three now, oh well 🤷

This should be good now!

Note to self: Use tests.nixpkgs-check-by-name in the commit message, that way ofborg should build it by default.

@infinisil infinisil changed the title nixpkgs-check-by-name: Fix for symlinked tempdirs tests.nixpkgs-check-by-name: Fix for symlinked tempdirs Sep 11, 2023
@infinisil infinisil added this to the RFC 140 milestone Sep 11, 2023
@infinisil
Copy link
Member Author

@ofborg build tests.nixpkgs-check-by-name

@infinisil
Copy link
Member Author

There are some ofborg failures but those are unrelated, they reference something that was removed a while ago, see NixOS/ofborg#650

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 254435 run on aarch64-darwin 1

1 package built:
  • tests.nixpkgs-check-by-name
$ nixpkgs-check-by-name nixpkgs/
Validated successfully

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 12, 2023
@infinisil infinisil merged commit dc194ec into NixOS:master Sep 12, 2023
@infinisil infinisil deleted the fix-by-name-check-on-darwin branch September 12, 2023 14:50
infinisil added a commit to tweag/nixpkgs that referenced this pull request Sep 22, 2023
github-actions bot pushed a commit that referenced this pull request Sep 23, 2023
This was an oversight in #254435

(cherry picked from commit 1fe58cb)
@nixos-discourse
Copy link

infinisil added a commit to NixOS/nixpkgs-vet that referenced this pull request Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants