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

Build failure: nodePackages_latest.nodejs #355919

Closed
JohnRTitor opened this issue Nov 14, 2024 · 22 comments · Fixed by #356257 or nodejs/node#55908
Closed

Build failure: nodePackages_latest.nodejs #355919

JohnRTitor opened this issue Nov 14, 2024 · 22 comments · Fixed by #356257 or nodejs/node#55908
Labels

Comments

@JohnRTitor
Copy link
Contributor

Steps To Reproduce

Steps to reproduce the behavior:

  1. build nodePackages_latest.nodejs from master

Build log

Build Log
┃        >   ...
┃        > ok 4059 known_issues/test-url-parse-conformance
┃        >   ---
┃        >   duration_ms: 56.50900
┃        >   ...
┃        > ok 4060 known_issues/test-vm-function-declaration-uses-define
┃        >   ---
┃        >   duration_ms: 36.65000
┃        >   ...
┃        > ok 4061 known_issues/test-vm-timeout-escape-nexttick
┃        >   ---
┃        >   duration_ms: 806.78800
┃        >   ...
┃        >
┃        > Failed tests:
┃        > out/Release/node /build/node-v23.2.0/test/parallel/test-os.js
┃        > make: *** [Makefile:558: test-ci-js] Error 1
┃        For full logs, run 'nix log /nix/store/ahgdyngidcphsmb7zn5b1lny589pyjdc-nodejs-23.2.0.drv'.

Additional context

Found it while building zed-editor on master.

Metadata

❯ nix-info -m

  • system: "x86_64-linux"
  • host os: Linux 6.11.7-cachyos, NixOS, 24.11 (Vicuna), 24.11.20241109.76612b1
  • multi-user?: yes
  • sandbox: yes
  • version: nix-env (Nix) 2.24.10
  • nixpkgs: /nix/store/gg86rfp39vc7chqsszk32q7995hz4943-source

Notify maintainers

@aduh95 @Ma27


Note for maintainers: Please tag this issue in your PR.


Add a 👍 reaction to issues you find important.

@JohnRTitor JohnRTitor added the 0.kind: build failure A package fails to build label Nov 14, 2024
@aduh95
Copy link
Contributor

aduh95 commented Nov 14, 2024

Failed tests:
┃ > out/Release/node /build/node-v23.2.0/test/parallel/test-os.js

Could you share the output of this test?

@JohnRTitor
Copy link
Contributor Author

Could you point me how to do that? I am not familiar with nodePackages, just got the log from the Nix build command.

@aduh95
Copy link
Contributor

aduh95 commented Nov 14, 2024

As it says on the output:

For full logs, run 'nix log /nix/store/ahgdyngidcphsmb7zn5b1lny589pyjdc-nodejs-23.2.0.drv'.

From there, you can search for not ok (or for the test name, but I find not ok easier to remember).

@JohnRTitor
Copy link
Contributor Author

Got it.

not ok 2075 parallel/test-os
  ---
  duration_ms: 37.37300
  severity: fail
  exitcode: 1
  stack: |-
    node:os:317
        throw new ERR_SYSTEM_ERROR(ctx);
        ^
    
    SystemError [ERR_SYSTEM_ERROR]: A system error occurred: uv_os_setpriority returned EACCES (permission denied)
        at Object.setPriority (node:os:317:11)
        at Object.<anonymous> (/build/node-v23.2.0/test/parallel/test-os.js:87:6)
        at Module._compile (node:internal/modules/cjs/loader:1550:14)
        at Object..js (node:internal/modules/cjs/loader:1702:10)
        at Module.load (node:internal/modules/cjs/loader:1307:32)
        at Function._load (node:internal/modules/cjs/loader:1121:12)
        at TracingChannel.traceSync (node:diagnostics_channel:322:14)
        at wrapModuleLoad (node:internal/modules/cjs/loader:219:24)
        at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:170:5)
        at node:internal/main/run_main_module:36:49 {
      code: 'ERR_SYSTEM_ERROR',
      info: {
        errno: -13,
        code: 'EACCES',
        message: 'permission denied',
        syscall: 'uv_os_setpriority'
      },
      errno: [Getter/Setter],
      syscall: [Getter/Setter]
    }
    
    Node.js v23.2.0
  ...
ok 2076 parallel/test-node-output-vm
  ---
  duration_ms: 331.84100
  ...

@aduh95
Copy link
Contributor

aduh95 commented Nov 14, 2024

OK that's definitely interesting! Could you confirm if the test passes if you revert nodejs/node@a094a81?

@LiviaMedeiros
Copy link

Does niceness on NixOS work differently than on most Unix systems? The os.constants.priority.PRIORITY_LOW is 19, there should be no numbers higher than that, and setting priority on same or lower level should be fine.

If it's indeed different, what's the returned value of require('node:os').type() on NixOS systems?

JohnRTitor added a commit to JohnRTitor/nixpkgs that referenced this issue Nov 15, 2024
JohnRTitor added a commit to JohnRTitor/nixpkgs that referenced this issue Nov 15, 2024
JohnRTitor added a commit to JohnRTitor/nixpkgs that referenced this issue Nov 15, 2024
@JohnRTitor
Copy link
Contributor Author

OK that's definitely interesting! Could you confirm if the test passes if you revert nodejs/node@a094a81?

Well this patch does not apply. JohnRTitor@24c5cc0

@aduh95
Copy link
Contributor

aduh95 commented Nov 15, 2024

Well this patch does not apply. JohnRTitor@24c5cc0

Ah right, my bad, that commit didn't make it into 23.2.0.
I reckon we have this test disabled on macOS, the comment says it's sandbox related:

# Disable tests that don’t work under macOS sandbox.
"test-macos-app-sandbox"
"test-os"

In any case, we should probably amend the Node.js test to not fail on EACCES

@JohnRTitor
Copy link
Contributor Author

Nice, can we cherry-pick the commit here or will it be part of a minor patch release?

@JohnRTitor
Copy link
Contributor Author

Looks like this does not directly apply either, we will either need to rebase and vendor the patch or disable the test, while we wait for a release.

@aduh95
Copy link
Contributor

aduh95 commented Nov 15, 2024

Well you would need to first apply nodejs/node@a094a81 and confirm it's still an issue with that patch applies, then cherry-pick my commit on top of it. It should make its way to a future minor or patch release yes.

@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 15, 2024

Does niceness on NixOS work differently than on most Unix systems?

I don't think it is. But I am not entirely sure.

If it's indeed different, what's the returned value of require('node:os').type() on NixOS systems?

@LiviaMedeiros

image

EDIT: I tried running node seperately on my system, it assigns 16 nice value to the node interpreter.

@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 15, 2024

Well you would need to first apply nodejs/node@a094a81

I mean this fails to apply. 🥲

diff --git a/pkgs/development/web/nodejs/v23.nix b/pkgs/development/web/nodejs/v23.nix
index a34e53375e69..6a5b7626eb54 100644
--- a/pkgs/development/web/nodejs/v23.nix
+++ b/pkgs/development/web/nodejs/v23.nix
@@ -53,5 +53,10 @@ buildNodejs {
       hash = "sha256-gmIyiSyNzC3pClL1SM2YicckWM+/2tsbV1xv2S3d5G0=";
       revert = true;
     })
+    # possible fix for https://github.com/NixOS/nixpkgs/issues/355919
+    (fetchpatch2 {
+      url = "https://github.com/nodejs/node/commit/a094a8166cd772f89e92b5deef168e5e599fa815.patch?full_index=1";
+      hash = "sha256-Jwf5D/QvoKNN36IqWKnWYXIpHdkm/o1rmq+G+n05EG0=";
+    })
   ];
 }

@aduh95
Copy link
Contributor

aduh95 commented Nov 15, 2024

Well you would need to first apply nodejs/node@a094a81

I mean this fails to apply. 🥲

diff --git a/pkgs/development/web/nodejs/v23.nix b/pkgs/development/web/nodejs/v23.nix
index a34e53375e69..6a5b7626eb54 100644
--- a/pkgs/development/web/nodejs/v23.nix
+++ b/pkgs/development/web/nodejs/v23.nix
@@ -53,5 +53,10 @@ buildNodejs {
       hash = "sha256-gmIyiSyNzC3pClL1SM2YicckWM+/2tsbV1xv2S3d5G0=";
       revert = true;
     })
+    # possible fix for https://github.com/NixOS/nixpkgs/issues/355919
+    (fetchpatch2 {
+      url = "https://github.com/nodejs/node/commit/a094a8166cd772f89e92b5deef168e5e599fa815.patch?full_index=1";
+      hash = "sha256-Jwf5D/QvoKNN36IqWKnWYXIpHdkm/o1rmq+G+n05EG0=";
+    })
   ];
 }

You need to fix the hash, you should get a different hash depending if you pass revert=true or not

@LiviaMedeiros
Copy link

EDIT: I tried running node seperately on my system, it assigns 16 nice value to the node interpreter.

Yep, this seems to be the issue. Before nodejs/node@a094a81 the test niceness was hardcoded to 10; and process can't lower its niceness unless you're root.
After applying that commit, it will check current priority and try 19 which should not cause EACCES.

@aduh95
Copy link
Contributor

aduh95 commented Nov 15, 2024

The hash you should be using is sha256-5FZfozYWRa1ZI/f+e+xpdn974Jg2DbiHbua13XUQP5E= for https://github.com/nodejs/node/commit/a094a8166cd772f89e92b5deef168e5e599fa815.patch?full_index=1

@JohnRTitor
Copy link
Contributor Author

Yeah I was just building the package. Looks like we need both nodejs/node@a094a81 and nodejs/node@f60cc58 for the test failure to be fixed.

@aduh95
Copy link
Contributor

aduh95 commented Nov 15, 2024

Can you clarify what's the behavior without nodejs/node@f60cc58? Same error as without nodejs/node@a094a81?

@JohnRTitor
Copy link
Contributor Author

The test indeed failed with just nodejs/node@a094a81 applied and needed nodejs/node@f60cc58 to work.

@LiviaMedeiros
Copy link

I guess we should indeed skip this test in case of EACCES since it identifies just as Linux, however normally this error shouldn't happen.

Does the following JS snippet throw?

const os = require('node:os');
console.log(`initial priority: ${os.getPriority()}`);
console.log(`setting priority: ${os.constants.priority.PRIORITY_LOW}`);
os.setPriority(os.constants.priority.PRIORITY_LOW);
console.log(`new priority: ${os.getPriority()}`);

Or this in command line?

nice
renice --priority 19 $$
nice

@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 18, 2024

image

Same thing with v23.1.0

❯  nice
renice --priority 19 $$
nice
16
30272 (process ID) old priority 16, new priority 19
19

@LiviaMedeiros
Copy link

LiviaMedeiros commented Nov 18, 2024

@JohnRTitor Thanks!
It seems that there's nothing out of ordinary in priority handling on NixOS. The test fix wasn't working, because the comparison was other way around. 😅 I'll open a PR with proper followup fix ASAP.

The patch is https://github.com/nodejs/node/commit/7f40deae6fee1534512ce25b25f9b89ec7e96aee.patch?full_index=1, it can be be applied on top of nodejs/node@a094a81 instead of nodejs/node@f60cc58.

nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Nov 20, 2024
PR-URL: #55908
Fixes: NixOS/nixpkgs#355919
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 2024
PR-URL: nodejs#55908
Fixes: NixOS/nixpkgs#355919
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Nov 26, 2024
PR-URL: nodejs#55908
Fixes: NixOS/nixpkgs#355919
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit to nodejs/node that referenced this issue Nov 26, 2024
PR-URL: #55908
Fixes: NixOS/nixpkgs#355919
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit to nodejs/node that referenced this issue Dec 10, 2024
PR-URL: #55908
Fixes: NixOS/nixpkgs#355919
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants