GeezerVison

CI: https://ci.nodejs.org/job/node-test-pull-request/43042/

CI: https://ci.nodejs.org/job/node-test-pull-request/43052/

CI: https://ci.nodejs.org/job/node-test-pull-request/43055/

There seem to be some relevant test failures on Windows.

cc: @nodejs/platform-windows @nodejs/fs

May be libuv/libuv#3193 ?
@RaisinTen

We could fix the EPERM -> EACCES stuff in the tests but I think it would be semver-major.

Also, there are two other different failures on Windows that need to be investigated:

1. node/test/parallel/test-os.js

   Line 215 in d4d0b09

   assert.strictEqual(pwd.uid, -1);

   pwd.uid is 4294967295 instead of -1. The culprit here seems to be libuv/libuv@f3e0bff.

2. node/test/parallel/test-timers-immediate-unref-nested-once.js

   Line 8 in d4d0b09

   setImmediate(common.mustNotCall()).unref();

   The callback is called when it should not. The offending commit is libuv/libuv@939a056.

cc: @nodejs/libuv @vtjnash

Yes, I can confirm that libuv/libuv#3193 caused the EPERM and EACCES mismatches and yes, this change should be labelled as semver-major because other modules like https://github.com/isaacs/rimraf also depends on the error code being EPERM instead of EACCES.

cc @isaacs

Yes, I can confirm that libuv/libuv#3193 caused the EPERM and EACCES mismatches and yes, this change should be labelled as semver-major because other modules like https://github.com/isaacs/rimraf also depends on the error code being EPERM instead of EACCES.

This means we will be unable to update libuv in LTS to this or subsequent versions.

This patch

diff --git a/deps/uv/src/win/core.c b/deps/uv/src/win/core.c
index c08dedc47c..cfd52e5148 100644
--- a/deps/uv/src/win/core.c
+++ b/deps/uv/src/win/core.c
@@ -401,7 +401,6 @@ int uv_loop_fork(uv_loop_t* loop) {
 static int uv__loop_alive(const uv_loop_t* loop) {
   return uv__has_active_handles(loop) ||
          uv__has_active_reqs(loop) ||
-         loop->pending_reqs_tail != NULL ||
          loop->endgame_handles != NULL;
 }

fixes the third issue (point 2 in my previous comment) but I'm not sure if it hides another issue/bug.

This means we will be unable to update libuv in LTS to this or subsequent versions.

@richardlau is that something we are okay with or should we revert the change in the libuv repo?

621ae7f could land on a separate PR, as it is not semver-major.

We would need to either squash the commits when merging, or add a first commit that disables the test that the update break, or make an exception (see #42356 (comment)).

This means we will be unable to update libuv in LTS to this or subsequent versions.

@richardlau is that something we are okay with or should we revert the change in the libuv repo?

On the one hand if we say "we'll never update libuv again in any of the existing LTS releases" then that's less work for releasers but I don't feel comfortable in making that statement. We could float patches but that's not particularly friendly to others who build their own packages of Node.js (Linux distros, homebrew etc) who dynamically link against a shared libuv library.

My question is, given the change would cause ecosystem breakage, is it worth it?

621ae7f could land on a separate PR, as it is not semver-major.

It could, but it does not make sense without the libuv update.

My question is, given the change would cause ecosystem breakage, is it worth it?

If it breaks things as popular as rimraf, I'd say it's not.

Please approve again. I will run CI again tomorrow when the embargo ends. Thanks.

CI: https://ci.nodejs.org/job/node-test-pull-request/47685/

CI: https://ci.nodejs.org/job/node-test-pull-request/47707/

CI: https://ci.nodejs.org/job/node-test-pull-request/47715/

CI: https://ci.nodejs.org/job/node-test-pull-request/47716/

Landed in 3a81b47...28bf031.