Skip to content

nodejs: fix checkPhase with sandbox=relaxed on Darwin #425699

Merged
fabianhjr merged 3 commits intoNixOS:stagingfrom
al3xtjames:node-darwin
Aug 30, 2025
Merged

nodejs: fix checkPhase with sandbox=relaxed on Darwin #425699
fabianhjr merged 3 commits intoNixOS:stagingfrom
al3xtjames:node-darwin

Conversation

@al3xtjames
Copy link
Contributor

@al3xtjames al3xtjames commented Jul 16, 2025 •
edited
Loading

The following tests fail on Darwin with the sandbox enabled:

not ok 2612 parallel/test-runner-output
not ok 3053 parallel/test-tls-get-ca-certificates-system
not ok 3054 parallel/test-tls-get-ca-certificates-system-without-flag
not ok 3363 parallel/test-watch-file-shared-dependency
not ok 4057 parallel/test-runner-complex-dependencies
not ok 4058 parallel/test-runner-global-setup-watch-mode
not ok 4228 sequential/test-watch-mode-watch-flags

Node.js uses Security.framework to read the system CA certificates from the system keychain on Darwin. Fix the tls-get-ca-certificates-system tests by adding the files and Mach services used by Security.framework to the sandbox profile. Also allow the FSEvents Mach service as the runner and watch tests use FSEvents on Darwin.

Also patch test-macos-app-sandbox to use codesign from sigtool, which allows it to work with the sandbox enabled.

Fixes: #423244

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • Nixpkgs 25.11 Release Notes (or backporting 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 25.05 NixOS Release notes)
    • (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, pkgs/README.md, maintainers/README.md and other contributing documentation in corresponding paths.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jul 16, 2025
@nix-owners nix-owners bot requested a review from aduh95 July 16, 2025 06:45
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment labels Jul 16, 2025
@al3xtjames al3xtjames changed the title node: fix checkPhase with sandbox=relaxed on Darwin node: fix checkPhase with sandbox=relaxed on Darwin Jul 16, 2025
@aduh95
Copy link
Contributor

aduh95 commented Jul 16, 2025 •
edited
Loading

Can you fix the commit message to use nodejs: instead of node: please? And ideally replace Node with Node.js

@al3xtjames al3xtjames changed the title node: fix checkPhase with sandbox=relaxed on Darwin nodejs: fix checkPhase with sandbox=relaxed on Darwin Jul 16, 2025
@aduh95 aduh95 mentioned this pull request Jul 18, 2025
Merged
13 tasks
ofalvai
ofalvai approved these changes Jul 23, 2025
View reviewed changes
Copy link
Contributor

@ofalvai ofalvai left a comment

Choose a reason for hiding this comment

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

Thank you for investigating this long-standing issue 🚀

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. 2.status: merge conflict This PR has merge conflicts with the target branch labels Jul 23, 2025
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 29, 2025
aduh95
aduh95 approved these changes Jul 29, 2025
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I'm not affected by the test failures, so I can't confirm it solves it, but the changes LGTM

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Jul 29, 2025
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 7, 2025
@aduh95
Copy link
Contributor

aduh95 commented Aug 13, 2025

@al3xtjames any chance you could fix the conflicts here please?

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. and removed 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. labels Aug 14, 2025
@al3xtjames al3xtjames mentioned this pull request Aug 19, 2025
Merged
13 tasks
@aduh95
Copy link
Contributor

aduh95 commented Aug 19, 2025

Any chance we could get this merged? /cc @FliegendeWurst @dasJ @drupol

@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 28, 2025
@aduh95
Copy link
Contributor

aduh95 commented Aug 30, 2025

@al3xtjames can you rebase to fix the conflicts please?

@al3xtjames
nodejs: drop v8 instrumentation patch
cbb3c85 
This is no longer needed now that apple-sdk_11 is the default SDK.
@al3xtjames
nodejs: use sigtool's codesign in test-macos-app-sandbox
1bc6e6c 
test-macos-app-sandbox uses the system-provided codesign binary
(/usr/bin/codesign) to apply entitlements to an app bundle. This fails
in the sandbox as /usr/bin/codesign is not accessible. Patch the test to
instead use the codesign binary from sigtool. The test was updated to
pass the executable path to codesign as sigtool can't handle the bundle
path.
@al3xtjames
nodejs: fix checkPhase with sandbox=relaxed on Darwin
8362b18 
The following tests fail on Darwin with the sandbox enabled [1]:

not ok 2612 parallel/test-runner-output
not ok 3053 parallel/test-tls-get-ca-certificates-system
not ok 3054 parallel/test-tls-get-ca-certificates-system-without-flag
not ok 3363 parallel/test-watch-file-shared-dependency
not ok 4057 parallel/test-runner-complex-dependencies
not ok 4058 parallel/test-runner-global-setup-watch-mode
not ok 4228 sequential/test-watch-mode-watch-flags

Node.js uses Security.framework to read the system CA certificates from
the system keychain on Darwin. Fix the tls-get-ca-certificates-system
tests by adding the files and Mach services used by Security.framework
to the sandbox profile. Also allow the FSEvents Mach service as the
runner and watch tests use FSEvents on Darwin.

[1]: https://gist.github.com/al3xtjames/0eb3c30d37c1ebab99968c62ee544300
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 30, 2025
@aduh95
Copy link
Contributor

aduh95 commented Aug 30, 2025

Any chance we could get this merged? /cc @dasJ @fabianhjr

@fabianhjr
Copy link
Member

fabianhjr commented Aug 30, 2025

Can't test on darwin but going by the two previous approvals

@fabianhjr fabianhjr merged commit 45859bf into NixOS:staging Aug 30, 2025
28 of 31 checks passed
@aduh95
Copy link
Contributor

aduh95 commented Aug 31, 2025

@al3xtjames do you think the failure in https://hydra.nixos.org/build/306435782 could be related to this PR?

@aduh95 aduh95 mentioned this pull request Aug 31, 2025
Merged
13 tasks
@al3xtjames
Copy link
Contributor Author

al3xtjames commented Aug 31, 2025

Hmm, e2e1617 builds fine on my local machine. I can reproduce the failure if I comment out ++ gypPatches here though: https://github.com/NixOS/nixpkgs/blob/staging-next/pkgs/development/web/nodejs/v22.nix#L56

@vcunat
Copy link
Member

vcunat commented Aug 31, 2025 •
edited
Loading

I think that darwin builders on hydra.nixos.org are unsandboxed. (I'm trying to stay away from darwin-specific stuff.)

@aduh95
Copy link
Contributor

aduh95 commented Aug 31, 2025

I've opened nodejs/gyp-next#311 to hopefully fix the error

@vcunat
Copy link
Member

vcunat commented Sep 1, 2025

So we now apply this to staging-next's nodejs* as a patch conditional for darwin?

@aduh95
Copy link
Contributor

aduh95 commented Sep 1, 2025

I've opened #439129

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 approved these changes

+1 more reviewer

@ofalvai ofalvai ofalvai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: darwin Running or building packages on Darwin 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@al3xtjames @aduh95 @fabianhjr @vcunat @ofalvai