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

nodejs: fix sandboxed build on darwin #262124

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Conversation

tie
Copy link
Member

@tie tie commented Oct 19, 2023

Description of changes

This change fixes sandboxed build on Darwin for Node.js.

Fixes #261820

Note: wait for upstream PR before merging.
The fix has been merged upstream, this PR backports it.

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 the 6.topic: darwin Running or building packages on Darwin label Oct 19, 2023
@tie tie force-pushed the node-darwin-sandbox branch 2 times, most recently from 40b8f0f to ede887d Compare October 20, 2023 15:47
@tie tie force-pushed the node-darwin-sandbox branch 5 times, most recently from 20280ac to 672cde8 Compare October 21, 2023 21:15
@tie tie marked this pull request as ready for review October 21, 2023 21:18
@tie tie force-pushed the node-darwin-sandbox branch 2 times, most recently from c75155b to 6393610 Compare October 23, 2023 15:55
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@tie tie force-pushed the node-darwin-sandbox branch 2 times, most recently from 86e01c6 to c2b340b Compare March 24, 2024 08:00
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 24, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024
@tie
Copy link
Member Author

tie commented Jul 3, 2024

OK, I can split these commits into another PR. Edit: done, these commits are now in separate PR.

@aduh95
Copy link
Contributor

aduh95 commented Jul 3, 2024

not ok 3036 parallel/test-tls-env-bad-extra-ca
  ---
  duration_ms: 84.14600
  severity: fail
  exitcode: 1
  stack: |-
    node:events:497
          throw er; // Unhandled 'error' event
          ^
    
    Error: spawn /build/node-v20.15.0/out/Release/node (deleted) ENOENT
        at ChildProcess._handle.onexit (node:internal/child_process:286:19)
        at onErrorNT (node:internal/child_process:484:16)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
    Emitted 'error' event on ChildProcess instance at:
        at ChildProcess._handle.onexit (node:internal/child_process:292:12)
        at onErrorNT (node:internal/child_process:484:16)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
      errno: -2,
      code: 'ENOENT',
      syscall: 'spawn /build/node-v20.15.0/out/Release/node (deleted)',
      path: '/build/node-v20.15.0/out/Release/node (deleted)',
      spawnargs: [ '/build/node-v20.15.0/test/parallel/test-tls-env-bad-extra-ca.js' ]
    }
    
    Node.js v20.15.0
…
not ok 3043 parallel/test-tls-enable-trace
  ---
  duration_ms: 87.46300
  severity: fail
  exitcode: 1
  stack: |-
    (node:129709) internal/test/binding: These APIs are for internal testing only. Do not use them.
    (Use `node --trace-warnings ...` to show where the warning was created)
    node:events:497
          throw er; // Unhandled 'error' event
          ^
    
    Error: spawn /build/node-v20.15.0/out/Release/node (deleted) ENOENT
        at ChildProcess._handle.onexit (node:internal/child_process:286:19)
        at onErrorNT (node:internal/child_process:484:16)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
    Emitted 'error' event on ChildProcess instance at:
        at ChildProcess._handle.onexit (node:internal/child_process:292:12)
        at onErrorNT (node:internal/child_process:484:16)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
      errno: -2,
      code: 'ENOENT',
      syscall: 'spawn /build/node-v20.15.0/out/Release/node (deleted)',
      path: '/build/node-v20.15.0/out/Release/node (deleted)',
      spawnargs: [
        '--expose-internals',
        '/build/node-v20.15.0/test/parallel/test-tls-enable-trace.js',
        'test'
      ]
    }
    
    Node.js v20.15.0
…
not ok 3070 parallel/test-runner-cli
  ---
  duration_ms: 2773.80000
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:126
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    
    1 !== 0
    
        at Object.<anonymous> (/build/node-v20.15.0/test/parallel/test-runner-cli.js:318:10)
        at Module._compile (node:internal/modules/cjs/loader:1358:14)
        at Module._extensions..js (node:internal/modules/cjs/loader:1416:10)
        at Module.load (node:internal/modules/cjs/loader:1208:32)
        at Module._load (node:internal/modules/cjs/loader:1024:12)
        at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:174:12)
        at node:internal/main/run_main_module:28:49 {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 1,
      expected: 0,
      operator: 'strictEqual'
    }
    
    Node.js v20.15.0
  ...
not ok 3071 parallel/test-runner-reporters
  ---
  duration_ms: 2621.54600
  severity: fail
  exitcode: 1
  stack: |-
        …
        not ok 14 - should support a custom ESM reporter from node_modules
          ---
          duration_ms: 20.931876
          location: '/build/node-v20.15.0/test/parallel/test-runner-reporters.js:115:3'
          failureType: 'testCodeFailure'
          error: "Cannot read properties of null (reading 'toString')"
          code: 'ERR_TEST_FAILURE'
          name: 'TypeError'
          stack: |-
            TestContext.<anonymous> (/build/node-v20.15.0/test/parallel/test-runner-reporters.js:119:37)
            Test.runInAsyncScope (node:async_hooks:206:9)
            Test.run (node:internal/test_runner/test:764:25)
            Test.start (node:internal/test_runner/test:668:17)
            node:internal/test_runner/test:1104:71
            node:internal/per_context/primordials:487:82
            new Promise (<anonymous>)
            new SafePromise (node:internal/per_context/primordials:455:29)
            node:internal/per_context/primordials:487:9
            Array.map (<anonymous>)
          ...
        # Subtest: should throw when reporter setup throws asynchronously
        not ok 15 - should throw when reporter setup throws asynchronously
          ---
          duration_ms: 18.085905
          location: '/build/node-v20.15.0/test/parallel/test-runner-reporters.js:126:3'
          failureType: 'testCodeFailure'
          error: |-
            Expected values to be strictly equal:
            
            null !== 7
            
          code: 'ERR_ASSERTION'
          name: 'AssertionError'
          expected: 7
          actual: ~
          operator: 'strictEqual'
          stack: |-
            TestContext.<anonymous> (/build/node-v20.15.0/test/parallel/test-runner-reporters.js:132:12)
            Test.runInAsyncScope (node:async_hooks:206:9)
            Test.run (node:internal/test_runner/test:764:25)
            Test.start (node:internal/test_runner/test:668:17)
            node:internal/test_runner/test:1104:71
            node:internal/per_context/primordials:487:82
            new Promise (<anonymous>)
            new SafePromise (node:internal/per_context/primordials:455:29)
            node:internal/per_context/primordials:487:9
            Array.map (<anonymous>)
          ...
        # Subtest: should throw when reporter errors
        not ok 16 - should throw when reporter errors
          ---
          duration_ms: 13.274049
          location: '/build/node-v20.15.0/test/parallel/test-runner-reporters.js:138:3'
          failureType: 'testCodeFailure'
          error: |-
            Expected values to be strictly equal:
            
            null !== 7
            
          code: 'ERR_ASSERTION'
          name: 'AssertionError'
          expected: 7
          actual: ~
          operator: 'strictEqual'
          stack: |-
            TestContext.<anonymous> (/build/node-v20.15.0/test/parallel/test-runner-reporters.js:142:12)
            Test.runInAsyncScope (node:async_hooks:206:9)
            Test.run (node:internal/test_runner/test:764:25)
            Test.start (node:internal/test_runner/test:668:17)
            node:internal/test_runner/test:1104:71
            node:internal/per_context/primordials:487:82
            new Promise (<anonymous>)
            new SafePromise (node:internal/per_context/primordials:455:29)
            node:internal/per_context/primordials:487:9
            Array.map (<anonymous>)
          ...
        # Subtest: should throw when reporter errors asynchronously
        not ok 17 - should throw when reporter errors asynchronously
          ---
          duration_ms: 10.379198
          location: '/build/node-v20.15.0/test/parallel/test-runner-reporters.js:148:3'
          failureType: 'testCodeFailure'
          error: |-
            Expected values to be strictly equal:
            
            null !== 7
            
          code: 'ERR_ASSERTION'
          name: 'AssertionError'
          expected: 7
          actual: ~
          operator: 'strictEqual'
          stack: |-
            TestContext.<anonymous> (/build/node-v20.15.0/test/parallel/test-runner-reporters.js:153:12)
            Test.runInAsyncScope (node:async_hooks:206:9)
            Test.run (node:internal/test_runner/test:764:25)
            Test.start (node:internal/test_runner/test:668:17)
            node:internal/test_runner/test:1104:71
            node:internal/per_context/primordials:487:82
            new Promise (<anonymous>)
            new SafePromise (node:internal/per_context/primordials:455:29)
            node:internal/per_context/primordials:487:9
            Array.map (<anonymous>)
          ...
        # Subtest: should support stdout as a destination with spec reporter
        not ok 18 - should support stdout as a destination with spec reporter
          ---
          duration_ms: 7.689268
          location: '/build/node-v20.15.0/test/parallel/test-runner-reporters.js:159:3'
          failureType: 'testCodeFailure'
          error: "Cannot read properties of null (reading 'toString')"
          code: 'ERR_TEST_FAILURE'
          name: 'TypeError'
          stack: |-
            TestContext.<anonymous> (/build/node-v20.15.0/test/parallel/test-runner-reporters.js:164:37)
            Test.runInAsyncScope (node:async_hooks:206:9)
            Test.run (node:internal/test_runner/test:764:25)
            Test.start (node:internal/test_runner/test:668:17)
            node:internal/test_runner/test:1104:71
            node:internal/per_context/primordials:487:82
            new Promise (<anonymous>)
            new SafePromise (node:internal/per_context/primordials:455:29)
            node:internal/per_context/primordials:487:9
            Array.map (<anonymous>)
          ...
        1..18
    not ok 1 - node:test reporters
      ---
      duration_ms: 2271.202088
      type: 'suite'
      location: '/build/node-v20.15.0/test/parallel/test-runner-reporters.js:15:1'
      failureType: 'subtestsFailed'
      error: '5 subtests failed'
      code: 'ERR_TEST_FAILURE'
      ...
    1..1
    # tests 18
    # suites 1
    # pass 13
    # fail 5
    # cancelled 0
    # skipped 0
    # todo 0
    # duration_ms 2312.85008
  ...

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024
@tie tie removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024
@tie
Copy link
Member Author

tie commented Jul 4, 2024

Seems to be some issue on staging branch, I’ll rebase again tomorrow.

@tie
Copy link
Member Author

tie commented Jul 5, 2024

Looks like ofborg-eval is still broken in staging (well, at least it was when I’ve started rebuilding this PR). Manual build succeeds.


Manual nix build --L -f . nodejs_18 nodejs_20 nodejs_22 succeeds on aarch64-darwin.


nixpkgs-review pr \
  --checkout commit \
  --package nodejs_18 \
  --package nodejs_20 \
  --package nodejs_22 \
  262124

Result of nixpkgs-review pr 262124 run on x86_64-linux 1

6 packages built:
  • nodejs_18
  • nodejs_18.libv8 (nodejs_18.libv8.libv8)
  • nodejs_20
  • nodejs_20.libv8 (nodejs_20.libv8.libv8)
  • nodejs_22
  • nodejs_22.libv8 (nodejs_22.libv8.libv8)

Result of nixpkgs-review pr 262124 run on aarch64-linux 1

6 packages built:
  • nodejs_18
  • nodejs_18.libv8 (nodejs_18.libv8.libv8)
  • nodejs_20
  • nodejs_20.libv8 (nodejs_20.libv8.libv8)
  • nodejs_22
  • nodejs_22.libv8 (nodejs_22.libv8.libv8)

@ofborg ofborg bot requested a review from aduh95 July 6, 2024 17:02
@tie
Copy link
Member Author

tie commented Jul 8, 2024

Rebased, ofborg is happy now except for LLVM build timeouts on Darwin, but it builds for me on aarch64-darwin (sandbox enabled), aarch64-linux and x86_64-linux. I don’t have x86_64-darwin to test though.

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.

It'd be nice to have the CI show it's working. Regardless of CI, we're certainly better with this patch than without it, so LGTM.

@tie tie added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jul 8, 2024
@tie tie requested a review from wegank July 8, 2024 11:02
@wegank wegank requested a review from marsam July 8, 2024 12:28
@aduh95
Copy link
Contributor

aduh95 commented Jul 8, 2024

We're unlikely to get a review from marsam: #306702

@wegank
Copy link
Member

wegank commented Jul 8, 2024

Oh, I forgot that, sorry...

@wegank wegank removed the request for review from marsam July 8, 2024 13:24
@wegank wegank merged commit fffa384 into NixOS:staging Jul 8, 2024
27 checks passed
@tie tie deleted the node-darwin-sandbox branch July 8, 2024 13:46
@tie tie mentioned this pull request Jul 15, 2024
13 tasks
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.

7 participants