GeezerVison

Review requested:

• @nodejs/actions
• @nodejs/gyp
• @nodejs/loaders
• @nodejs/security-wg
• @nodejs/startup
• @nodejs/tsc
• @nodejs/url

Ping @nodejs/releasers for visibility as this the backport of a semver minor into an LTS maintainance.

#50322 is only semver-minor because it adds a new dependency, by the way, which I don't think we expose, so it's technically internal.

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

simdjson cannot compile on fedora-last-latest-x64 https://ci.nodejs.org/job/node-test-commit-linux/62561/nodes=fedora-last-latest-x64/console

19:26:53 ../deps/simdjson/simdjson.cpp:16637:49: error: inlining failed in call to ‘always_inline’ ‘simdjson::error_code simdjson::fallback::{anonymous}::stage2::json_iterator::walk_document(V&) noexcept [with bool STREAMING = false; V = simdjson::fallback::{anonymous}::stage2::tape_builder]’: target specific option mismatch
19:26:53 16637 | simdjson_warn_unused simdjson_inline error_code json_iterator::walk_document(V &visitor) noexcept {
19:26:53       |                                                 ^~~~~~~~~~~~~
19:26:53 ../deps/simdjson/simdjson.cpp:17074:39: note: called from here
19:26:53 17074 |   return iter.walk_document<STREAMING>(builder);
19:26:53       |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
19:26:53 ../deps/simdjson/simdjson.cpp:16637:49: error: inlining failed in call to ‘always_inline’ ‘simdjson::error_code simdjson::fallback::{anonymous}::stage2::json_iterator::walk_document(V&) noexcept [with bool STREAMING = false; V = simdjson::fallback::{anonymous}::stage2::tape_builder]’: target specific option mismatch
19:26:53 16637 | simdjson_warn_unused simdjson_inline error_code json_iterator::walk_document(V &visitor) noexcept {
19:26:53       |                                                 ^~~~~~~~~~~~~
19:26:53 ../deps/simdjson/simdjson.cpp:17074:39: note: called from here
19:26:53 17074 |   return iter.walk_document<STREAMING>(builder);
19:26:53       |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~

I think this is likely due to the fact that we are still using gnu++17 for v20.x. I am not particularly confident in changing it to gnu++20 without breaking ABI. Another possible fix is to pretend that there's no optimization for GCC and not use always_inline, if that doesn't work, I feel that the best course of action is to not bother backporting this and just rewrite other commits to deal with the package json reader conflicts instead.

cc @lemire can you look?

Using inline instead of forcing it to always_inline in specific places seems to at least fix it for test-digitalocean-fedora40-x64-1, there could be a gcc bug somewhere but then we aren't in a position to update the toolchain, so it's better to just patch the source.

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

AIX is failing with

  AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
  + actual - expected

    Comparison {
  +   code: 'ERR_INVALID_PACKAGE_CONFIG',
  +   message: 'Invalid package config /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/fixtures/packages/is-dir/package.json.'
  -   code: 'MODULE_NOT_FOUND',
  -   message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/
    }
      at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/parallel/test-module-loading-error.js:87:8)
      at Module._compile (node:internal/modules/cjs/loader:1468:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1547:10)
      at Module.load (node:internal/modules/cjs/loader:1287:32)
      at Module._load (node:internal/modules/cjs/loader:1103:12)
      at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:180:12)
      at node:internal/main/run_main_module:28:49

I think that's probably caused by backporting #53502 without backporting #48605 and the test changes should just be reverted back to be AIX-specific again.

AIX is failing with

I think that's probably caused by backporting #53502 without backporting #48605 and the test changes should just be reverted back to be non-AIX-specific again.

#48605 is semver-major. #48477 had to include some code paths to not break AIX -- I think they're missing from this backport (based on what #48605 removed -- I would expect to see the removed code from that PR in this backport).

The test failure is coming from an earlier, unchanged line in the test case, which implies that as it stand this backport would be breaking.

Indeed, it seems #50322 was assuming that it could depend on code that already broke the special case for AIX and to backport it we need to bring that special case back. I am not quite sure how to rewrite the PR to bring back that special case, however... @anonrig @nodejs/platform-aix anyone knows?

Actually looking at libuv/libuv#2025 I think I know now - just checks if it's a directory for AIX and return exist: false in that case.

Actually looking at libuv/libuv#2025 I think I know now - just checks if it's a directory for AIX and return exist: false in that case.

I just had the chance to look. It seems your last commit is the correct approach. Thank you for following up and backporting this @joyeecheung

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

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

Only parallel/test-buffer-tostring-range fails on SmartOS, though it has been failing on v20.x-staging for some time...for example, yesterday's daily build on v20.x-staging: https://ci.nodejs.org/job/node-test-commit-smartos/58708/ and a month ago it was probably also failing there https://ci.nodejs.org/job/node-daily-v20.x-staging/414/ (SmartOS data was already cleaned up)

(By the way the state of v20.x-staging's CI doesn't look great either https://ci.nodejs.org/job/node-daily-v20.x-staging/445/)

Only parallel/test-buffer-tostring-range fails on SmartOS, though it has been failing on v20.x-staging for some time...for example, yesterday's daily build on v20.x-staging: https://ci.nodejs.org/job/node-test-commit-smartos/58708/ and a month ago it was probably also failing there https://ci.nodejs.org/job/node-daily-v20.x-staging/414/ (SmartOS data was already cleaned up)

(By the way the state of v20.x-staging's CI doesn't look great either https://ci.nodejs.org/job/node-daily-v20.x-staging/445/)

@nodejs/platform-smartos

Thanks for the heads up! I'm trying to create an issue in nodejs/node to specifically track looking into parallel.test-buffer-tostring-range failures on illumos, and assign myself and @bahamat, but for whatever reason I don't seem to able to use either Assignees or Labels in the UI, despite being a member of the @nodejs/platform-smartos group. How does one get that ability?

I'd like to include these commits in the release, we might need to include a fix for smartos but thats independent from this PR, so I'll go ahead and backport it.

reopening as I'm going to delay this backport and remove it from staging

Actually it was good that this is not going into v20.x alone, because the original PR caused some regressions and the fixes should also be backported:

#51269
#55777
#51097

-> #53294 this then leads to backporting #52135 which also caused a series of regressions, one of them fixed by #54224 which is semver-major. Another seems to be still present on the main branch #54367

I am now in a bind - if this is not backported, then all the subsequent commits related to require(esm) would be very prone to conflicts because of the scale of changes introduced by the original PR. But if this is backported it feels very hard to track down all the regressions and their fixes (I am also not very confident that they are all already fixed on main?)

I looked into the regressions caused by the original PR and the regressions caused by the commits that the fixes depend on, and I don't feel confident enough at this point to backport all of them to v20.x without destabilizing v20.x (it seems some regressions in the chain are still unfixed on the main branch?). So I'll go with another route of backporting as few commits as possible for require(esm) as that seems less risky.