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

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

Merged
merged 2 commits into from
Sep 12, 2023

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.

@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 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
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

@infinisil infinisil merged commit dc194ec into NixOS:master Sep 12, 2023
28 checks passed
@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

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-deal-with-waiting-for-the-channel-in-production-environments/33378/8

digtail pushed a commit to digtail/nixpkgs that referenced this pull request Sep 28, 2023
infinisil added a commit to NixOS/nixpkgs-check-by-name 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants