node icon indicating copy to clipboard operation
node copied to clipboard

tools: fix riscv64 build failed

Open luyahan opened this issue 1 year ago • 3 comments

fix https://github.com/nodejs/node/issues/52678

luyahan avatar May 08 '24 08:05 luyahan

Review requested:

  • [ ] @nodejs/gyp
  • [ ] @nodejs/v8-update

nodejs-github-bot avatar May 08 '24 08:05 nodejs-github-bot

I had a try with your branch with GCC10 from Ubuntu 20.04/riscv64 and run into an issue with a number of symbols when building the test code - is that expected and do you get the same failure? I don't think I've seen these before.

../deps/googletest/include/gtest/internal/gtest-port.h: In function 'int testing::internal::posix::RmDir(const char*)':
../deps/googletest/include/gtest/internal/gtest-port.h:2053:44: error: 'rmdir' was not declared in this scope; did you mean 'simdutf::internal::rmdir'?
../deps/googletest/include/gtest/internal/gtest-port.h: In function 'int testing::internal::posix::DoIsATTY(int)':
../deps/googletest/include/gtest/internal/gtest-port.h:2084:38: error: 'isatty' was not declared in this scope; did you mean 'simdutf::internal::isatty'?
../deps/googletest/include/gtest/internal/gtest-port.h: In function 'int testing::internal::posix::ChDir(const char*)':
../deps/googletest/include/gtest/internal/gtest-port.h:2113:44: error: 'chdir' was not declared in this scope; did you mean 'simdutf::internal::chdir'?
../deps/googletest/include/gtest/internal/gtest-port.h: In function 'int testing::internal::posix::Read(int, void*, unsigned int)':
../deps/googletest/include/gtest/internal/gtest-port.h:2135:27: error: 'read' was not declared in this scope; did you mean 'simdutf::internal::read'?
../deps/googletest/include/gtest/internal/gtest-port.h: In function 'int testing::internal::posix::Write(int, const void*, unsigned int)':
../deps/googletest/include/gtest/internal/gtest-port.h:2138:27: error: 'write' was not declared in this scope; did you mean 'simdutf::internal::write'?
../deps/googletest/include/gtest/internal/gtest-port.h: In function 'int testing::internal::posix::Close(int)':
../deps/googletest/include/gtest/internal/gtest-port.h:2140:35: error: 'close' was not declared in this scope; did you mean 'simdutf::internal::close'?

EDIT: I'm going to be away for the next 2½ weeks - so until June - so if I don't respond to any further queries on this, that will be why

sxa avatar May 10 '24 16:05 sxa

Why does that check for host_arch=="arm64" or target_arch=="arm64"?

andreas-schwab avatar May 23 '24 13:05 andreas-schwab

Why does that check for host_arch=="arm64" or target_arch=="arm64"?

Thanks. I had fix this.

luyahan avatar Jun 19 '24 08:06 luyahan

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

nodejs-github-bot avatar Aug 06 '24 15:08 nodejs-github-bot

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

LGTM - three failures - all passed on first re-run. First two are marked flakey:

  • https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu2204_sharedlibs_openssl31_x64/45075/testReport/(root)/benchmark/test_benchmark_crypto_/
  • https://ci.nodejs.org/job/node-test-commit-plinux/nodes=rhel8-ppc64le/54657/testReport/(root)/parallel/test_sqlite_/
  • https://ci.nodejs.org/job/node-test-commit-arm/nodes=ubuntu2004-arm64/53936/testReport/(root)/parallel/test_error_serdes_/

@luyahan Before I approve (it fixes the build failure for me on a recent gcc) is there a reason why this PR appears to add extra files into the x64 build as well? I would expect this fox to only be in riscv64 specific sections.

sxa avatar Aug 07 '24 16:08 sxa

@luyahan Hello,

can you add the repair code for the loong64 architecture? loong64 also needs this patch.

Thanks you.

        ['v8_target_arch=="loong64"', {
          'sources': [
            '<!@pymod_do_main(GN-scraper "<(V8_ROOT)/BUILD.gn"  "\\"v8_base_without_compiler.*?v8_enable_wasm_gdb_remote_debugging.*?v8_current_cpu == \\"loong64\\".*?sources \\+= ")',
          ],
          'conditions': [
            ['v8_enable_webassembly==1', {
              'conditions': [
                ['(_toolset=="host" and host_arch=="arm64" or _toolset=="target" and target_arch=="arm64") or (_toolset=="host" and host_arch=="loong64" or _toolset=="target" and target_arch=="loong64") or (_toolset=="host" and host_arch=="x64" or _toolset=="target" and target_arch=="x64")', {
                  'sources': [
                    '<(V8_ROOT)/src/trap-handler/handler-inside-posix.cc',
                    '<(V8_ROOT)/src/trap-handler/handler-outside-posix.cc',
                  ],
                }],
                ['(_toolset=="host" and host_arch=="x64" or _toolset=="target" and target_arch=="x64") and (OS=="linux")', {
                  'sources': [
                    '<(V8_ROOT)/src/trap-handler/handler-outside-simulator.cc',
                  ],
                }],
              ],
            }],
          ],
        }],

image

wojiushixiaobai avatar Oct 04 '24 13:10 wojiushixiaobai

Is the riscv build for 22 still broken waiting for this to be reviewed?

tuler avatar Oct 24 '24 21:10 tuler

@luyahan Before I approve (it fixes the build failure for me on a recent gcc) is there a reason why this PR appears to add extra files into the x64 build as well? I would expect this fox to only be in riscv64 specific sections.

@luyahan and @sxa Can you get this sorted out?

It would be great if Node.js supported Linux/RISC-V (64-bit) again.

benz0li avatar Nov 09 '24 06:11 benz0li

I had to pull this patch into Fedora/RISCV NodeJS 22 to get it building again. http://fedora.riscv.rocks/koji/buildinfo?buildID=343396

Could someone review this patch to unblock NodeJS 22?

davidlt avatar Nov 14 '24 10:11 davidlt

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

LGTM - three failures - all passed on first re-run. First two are marked flakey:

  • https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu2204_sharedlibs_openssl31_x64/45075/testReport/(root)/benchmark/test_benchmark_crypto_/
  • https://ci.nodejs.org/job/node-test-commit-plinux/nodes=rhel8-ppc64le/54657/testReport/(root)/parallel/test_sqlite_/
  • https://ci.nodejs.org/job/node-test-commit-arm/nodes=ubuntu2004-arm64/53936/testReport/(root)/parallel/test_error_serdes_/

@luyahan Before I approve (it fixes the build failure for me on a recent gcc) is there a reason why this PR appears to add extra files into the x64 build as well? I would expect this fox to only be in riscv64 specific sections.

There have condition ['v8_target_arch=="riscv64"',, so it don't add file into x64 build.

((_toolset=="host" and host_arch=="x64" or _toolset=="target" and target_arch=="x64")

The condition will be effective after the pr https://github.com/nodejs/node/pull/55848 When enable v8_simulator, v8_target_arch != target_arch.

luyahan avatar Nov 14 '24 11:11 luyahan

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

LGTM - three failures - all passed on first re-run. First two are marked flakey:

  • https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu2204_sharedlibs_openssl31_x64/45075/testReport/(root)/benchmark/test_benchmark_crypto_/
  • https://ci.nodejs.org/job/node-test-commit-plinux/nodes=rhel8-ppc64le/54657/testReport/(root)/parallel/test_sqlite_/
  • https://ci.nodejs.org/job/node-test-commit-arm/nodes=ubuntu2004-arm64/53936/testReport/(root)/parallel/test_error_serdes_/

@luyahan Before I approve (it fixes the build failure for me on a recent gcc) is there a reason why this PR appears to add extra files into the x64 build as well? I would expect this fox to only be in riscv64 specific sections.

Hi @sxa Could you have time to review this pr again?

luyahan avatar Nov 14 '24 11:11 luyahan

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

nodejs-github-bot avatar Nov 16 '24 10:11 nodejs-github-bot

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

nodejs-github-bot avatar Nov 18 '24 05:11 nodejs-github-bot

@luyahan Can you adjust the commit message as per the "Merging is blocked" notification above please?

sxa avatar Nov 18 '24 14:11 sxa

@luyahan Can you adjust the commit message as per the "Merging is blocked" notification above please?

You can ignore that -- the tooling for merging will take care of it.

richardlau avatar Nov 18 '24 15:11 richardlau

Landed in bdaa898ceaf61840d030ee83b2e9adf40973544d

nodejs-github-bot avatar Nov 18 '24 15:11 nodejs-github-bot