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

Add support for linux s390x gnu #5346

Merged
merged 1 commit into from Mar 27, 2024

Conversation

edlerd
Copy link
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 :)

Copy link

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
Contributor

We are interested in s390x support as well

@pavolloffay
Copy link
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
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
Contributor

I have submitted a similar #5350 for ppc64le

Copy link
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
Copy link
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

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

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
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

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
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

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
Copy link
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
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.

Copy link

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
29 checks passed
Copy link

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

@uweigand
Copy link

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!

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.

None yet

5 participants