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 ppc64le #5350
Add support for ppc64le #5350
Conversation
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some CI issues, please have a look
@lukastaegert could you please run the CI? I have pushed one commit to fix the issue. |
@lukastaegert could you please run the CI again? :) |
@lukastaegert I have filed #5354 for wasm-node to unblock our build. Any help is much appreciated |
@lukastaegert could you please run the CI again? |
I don't know how to solve the last build error. It seems similar to the one for s390x #5346 TODO: fix linker https://github.com/japaric/rust-cross#cross-compiling-with-cargo |
To my limited knowledge, these kind of errors usually mean that it tries to use the wrong linker (it actually says "linking with cc failed"). One solution sometimes is to use "zig", otherwise we need to try stuff. Cross-compilation is apparently non-trivial. |
Do you have any examples of using the "zig"? Do you mean smth like this https://github.com/rollup/rollup/pull/5350/files#diff-4871c3b2dc0b429294e4a447d2b362ca146660f04622cf459b2fb8d99bc55390R136 ? |
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@lukastaegert could you please enable the CI again :) |
Any help would be appreciated. Now I am stuck again. |
It seems we need a profoundly new cross-compilation process. I am no expert either (otherwise, I might just have fixed things). Here https://github.com/cross-rs/cross they claim to be able to compile for those targets, one would need to figure out if this can be married to napi-rs. Or maybe there are other ways to do cross-compilation. |
Given the success we've had with s390x here #5346, I think the same approach ought to work for ppc64le as well. This means:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I used my maintainer privileges and pushed the proposed changes and alas, IT WORKED! This looks good from my side now, and unless there are any concerns, I will merge and release this later today.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5350 +/- ##
=======================================
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. |
Thanks @uweigand I have tried cross build for ppc64le and it is working as expected |
Thanks again, @lukastaegert ! Will this be in the 4.13.2 release then? |
This PR has been released as part of rollup@4.13.2. You can test it via |
There were some issues with the folder name but it is released now and hopefully works. Apparently, Node calls it ppc64le while Rust likes the longer powerpc64le... I hope I resolved it correctly. |
Ah, I see. This looks good to me ... |
@@ -16,6 +16,7 @@ const bindingsByPlatformAndArch = { | |||
linux: { | |||
arm: { base: 'linux-arm-gnueabihf', musl: null }, | |||
arm64: { base: 'linux-arm64-gnu', musl: 'linux-arm64-musl' }, | |||
ppc64le: { base: 'linux-ppc64le-gnu', musl: null }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pavolloffay @lukastaegert
I guess this line and cpu
field in package.json need to be ppc64
instead of ppc64le
. process.arch
seems to return ppc64
.
https://github.com/konveyor/tackle2-ui/actions/runs/8150208242/job/22276088934#step:7:89
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you are right and process.arch
always returns ppc64
- which in practice means ppc64le as Node hasn't been supporting big-endian Linux for a while. In theory there is also an os.machine
interface that returns either ppc64
or ppc64le
, but it doesn't seem necessary to use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If anyone wants to make an educated PR to improve here, this would be very welcome! Otherwise, I can also have a look in the next days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR here: #5460
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sapphi-red, just wondering whether the native ppc64le rollup work fors you now? It looks like tackle2-ui downgraded to rollup v3 in the meantime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a ppc64le machine so I don't know 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spend some hours setting up emulated smoke tests for some environments including ppc64le and I can confirm it works now 😀
#5477
This issue has been resolved via #5460 as part of rollup@4.14.2. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Add support for ppc64le linux gnu target.
The PR is similar to #5346. Heads up I am not working with node ecosystem. I just need to build https://github.com/jaegertracing/jaeger-ui on ppc64le which gives me at the moment: