Skip to content

Add support for linux s390x gnu#5346

Merged
lukastaegert merged 1 commit intorollup:masterfrom
edlerd:add-s390x-native-build
Mar 27, 2024
Merged

Add support for linux s390x gnu#5346
lukastaegert merged 1 commit intorollup:masterfrom
edlerd:add-s390x-native-build

Conversation

@edlerd
Copy link
Copy Markdown
Contributor

@edlerd edlerd commented Jan 18, 2024

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Add support for s390x linux gnu target.

I would very much appreciate it if a new release with s390x support could be published after we get this PR merged :)

@vercel
Copy link
Copy Markdown

vercel bot commented Jan 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 26, 2024 4:08pm

@pavolloffay
Copy link
Copy Markdown
Contributor

We are interested in s390x support as well

@pavolloffay
Copy link
Copy Markdown
Contributor

@edlerd we are getting this error:

Your current platform "linux" and architecture "s390x" combination is not yet supported by the native Rollup build. Please use the WASM build "@rollup/wasm-node" instead.

Does the workaround with rollup/wasm-node work?

@edlerd
Copy link
Copy Markdown
Contributor Author

edlerd commented Jan 18, 2024

Does the workaround with rollup/wasm-node work?

We need it in combination with vite 5.0 -- which uses rollup 4. It seems to work with rollup/wasm-node on s390x at first, but then vite build crashes in our build with the below trace

memory access out of bounds (Note that you need plugins to import files that are not JavaScript)
:: file: /build/lxd/parts/lxd-ui/build/index.html
:: error during build:
:: RuntimeError: memory access out of bounds
::     at wasm://wasm/0063bf96:wasm-function[1558]:0x162080
::     at wasm://wasm/0063bf96:wasm-function[249]:0x104dc0
::     at wasm://wasm/0063bf96:wasm-function[1442]:0x15f2f9
::     at module.exports.parse (/build/lxd/parts/lxd-ui/build/node_modules/rollup/dist/wasm-node/bindings_wasm.js:135:14)
::     at exports.parseAsync (/build/lxd/parts/lxd-ui/build/node_modules/rollup/dist/native.js:5:2)
::     at parseAstAsync (file:///build/lxd/parts/lxd-ui/build/node_modules/rollup/dist/es/shared/parseAst.js:2148:29)
::     at Module.tryParseAsync (file:///build/lxd/parts/lxd-ui/build/node_modules/rollup/dist/es/shared/node-entry.js:13517:26)
::     at Module.setSource (file:///build/lxd/parts/lxd-ui/build/node_modules/rollup/dist/es/shared/node-entry.js:13098:46)
::     at ModuleLoader.addModuleSource (file:///build/lxd/parts/lxd-ui/build/node_modules/rollup/dist/es/shared/node-entry.js:17788:26)

I opened this PR to unblock the build. Hints on how to fix the build with rollup/wasm-node are highly welcome too :)

@pavolloffay pavolloffay mentioned this pull request Jan 19, 2024
9 tasks
@pavolloffay
Copy link
Copy Markdown
Contributor

I have submitted a similar #5350 for ppc64le

Copy link
Copy Markdown
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at CI, unfortunately it does not work yet.

@edlerd edlerd force-pushed the add-s390x-native-build branch from f0adc9b to 9bd40ad Compare January 19, 2024 17:06
@edlerd
Copy link
Copy Markdown
Contributor Author

edlerd commented Jan 19, 2024

Please have a look at CI, unfortunately it does not work yet.

Fixed the lint error, though no clue yet how to fix the build error.

@iblancasa
Copy link
Copy Markdown

I think this error message is related @edlerd:

 2024-01-19T19:12:21.892Z napi:build Run cargo build --release --target s390x-unknown-linux-gnu -p bindings_napi
/bin/sh: 1: zig: not found
2024-01-19T19:12:21.895Z napi:build Could not find zig on the PATH, fallback to normal linker

I cannot see that error message in the build for other architectures. ld seems to not be configured properly for cross-compilation... trying to link (I guess) x64 stuff with s390x stuff.

@uweigand
Copy link
Copy Markdown

uweigand commented Mar 8, 2024

We just ran into this issue as well - thanks for working on s390x support, @edlerd!

ld seems to not be configured properly for cross-compilation... trying to link (I guess) x64 stuff with s390x stuff.

Normally, cargo should be able to use the proper cross-linker. You need to set the environment variable

CARGO_TARGET_S390X_UNKNOWN_LINUX_GNU_LINKER=s390x-linux-gnu-gcc

I'm not very familiar with the napi build script, but from a quick glance it seems it should already attempt to set this up, but this doesn't appear to be working correctly (you can see in the error message that cargo just falls back to cc as linker). You might try just setting the above environment variable directly in the build-and-tests.yml rule.

@edlerd
Copy link
Copy Markdown
Contributor Author

edlerd commented Mar 26, 2024

Normally, cargo should be able to use the proper cross-linker. You need to set the environment variable

CARGO_TARGET_S390X_UNKNOWN_LINUX_GNU_LINKER=s390x-linux-gnu-gcc

Thanks for the suggestion, I added this just before calling the build (also rebased on current master branch). HTH

@uweigand
Copy link
Copy Markdown

I've now tried a cross-build x86_64 -> s390x using this PR and the linker environment variable, and this completed successfully for me.

@lukastaegert
Copy link
Copy Markdown
Member

This CI run actually looked quite good! Now the only missing thing is s390x-unknown-linux-gnu-strip: command not found. Of course we can live without stripping debug symbols for the initial effort, but they make the binary huge.

@uweigand
Copy link
Copy Markdown

This CI run actually looked quite good! Now the only missing thing is s390x-unknown-linux-gnu-strip: command not found. Of course we can live without stripping debug symbols for the initial effort, but they make the binary huge.

Ah, that should be s390x-linux-gnu-strip (without the unknown). That should be present as part of the standard Ubuntu cross-toolchain.

Signed-off-by: David Edler <david.edler@canonical.com>
@edlerd edlerd force-pushed the add-s390x-native-build branch from 6255d0a to 8ec9abf Compare March 26, 2024 16:07
@edlerd
Copy link
Copy Markdown
Contributor Author

edlerd commented Mar 26, 2024

This CI run actually looked quite good! Now the only missing thing is s390x-unknown-linux-gnu-strip: command not found. Of course we can live without stripping debug symbols for the initial effort, but they make the binary huge.

Ah, that should be s390x-linux-gnu-strip (without the unknown). That should be present as part of the standard Ubuntu cross-toolchain.

Thanks again for the hint, updated the PR accordingly.

Copy link
Copy Markdown
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks amazing now! Let's hope it actually works 😉
And thank you so much for sticking with it, I will make sure we can release it maybe as early as today.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.80%. Comparing base (23d5282) to head (8ec9abf).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5346   +/-   ##
=======================================
  Coverage   98.80%   98.80%           
=======================================
  Files         236      236           
  Lines        9423     9423           
  Branches     2398     2398           
=======================================
  Hits         9310     9310           
  Misses         48       48           
  Partials       65       65           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lukastaegert lukastaegert added this pull request to the merge queue Mar 27, 2024
Merged via the queue into rollup:master with commit c152806 Mar 27, 2024
@github-actions
Copy link
Copy Markdown

This PR has been released as part of rollup@4.13.1. You can test it via npm install rollup.

@uweigand
Copy link
Copy Markdown

This looks amazing now! Let's hope it actually works 😉 And thank you so much for sticking with it, I will make sure we can release it maybe as early as today.

Thank you very much for your support!

@sec sec mentioned this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants