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 ppc64le #5350

Merged
merged 7 commits into from Mar 28, 2024
Merged

Add support for ppc64le #5350

merged 7 commits into from Mar 28, 2024

Conversation

pavolloffay
Copy link
Contributor

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

#9 548.1 $ NODE_ENV=production REACT_APP_VSN_STATE=$(../../scripts/get-tracking-version.js) vite build
#9 553.5 /workspace/node_modules/rollup/dist/native.js:38
#9 553.5 	throw new Error(
#9 553.5 	      ^
#9 553.5 
#9 553.5 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.
#9 553.5 
#9 553.5 The following platform-architecture combinations are supported:
#9 553.5 android-arm
#9 553.5 android-arm64
#9 553.5 darwin-arm64
#9 553.5 darwin-x64
#9 553.5 linux-arm
#9 553.5 linux-arm64
#9 553.5 linux-arm64 (musl)
#9 553.5 linux-riscv64
#9 553.5 linux-x64
#9 553.5 linux-x64 (musl)
#9 553.5 win32-arm64
#9 553.5 win32-ia32
#9 553.5 win32-x64
#9 553.5 
#9 553.5 If this is important to you, please consider supporting Rollup to make a native build for your platform and architecture available.
#9 553.5     at Object.<anonymous> (/workspace/node_modules/rollup/dist/native.js:38:8)
#9 553.5     at Module._compile (node:internal/modules/cjs/loader:1356:14)
#9 553.5     at Module._extensions..js (node:internal/modules/cjs/loader:1414:10)
#9 553.5     at Module.load (node:internal/modules/cjs/loader:1197:32)
#9 553.5     at Module._load (node:internal/modules/cjs/loader:1013:12)
#9 553.5     at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:202:29)
#9 553.5     at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
#9 553.5     at async ModuleLoader.import (node:internal/modules/esm/loader:336:24)
#9 553.5 
#9 553.5 Node.js v18.19.0
#9 553.6 error Command failed with exit code 1.
#9 553.6 info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
#9 ERROR: process "/bin/sh -c yarn install --frozen-lockfile && cd ./packages/jaeger-ui && yarn build" did not complete successfully: exit code: 1
------
 > [4/4] RUN  yarn install --frozen-lockfile && cd ./packages/jaeger-ui && yarn build:
553.5     at Module._extensions..js (node:internal/modules/cjs/loader:1414:10)
553.5     at Module.load (node:internal/modules/cjs/loader:1197:32)
553.5     at Module._load (node:internal/modules/cjs/loader:1013:12)
553.5     at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:202:29)
553.5     at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
553.5     at async ModuleLoader.import (node:internal/modules/esm/loader:336:24)
553.5 
553.5 Node.js v18.19.0
553.6 error Command failed with exit code 1.
553.6 info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
------
WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load
Dockerfile:7
--------------------
   5 |     COPY . .
   6 |     
   7 | >>> RUN  yarn install --frozen-lockfile && cd ./packages/jaeger-ui && yarn build
   8 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c yarn install --frozen-lockfile && cd ./packages/jaeger-ui && yarn build" did not complete successfully: exit code: 1

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Copy link

vercel bot commented Jan 19, 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 28, 2024 5:37am

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.

There are still some CI issues, please have a look

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@pavolloffay
Copy link
Contributor Author

@lukastaegert could you please run the CI? I have pushed one commit to fix the issue.

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@pavolloffay
Copy link
Contributor Author

@lukastaegert could you please run the CI again? :)

@pavolloffay
Copy link
Contributor Author

@lukastaegert I have filed #5354 for wasm-node to unblock our build. Any help is much appreciated

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@pavolloffay
Copy link
Contributor Author

@lukastaegert could you please run the CI again?

@pavolloffay
Copy link
Contributor Author

pavolloffay commented Jan 22, 2024

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

@lukastaegert
Copy link
Member

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.

@pavolloffay
Copy link
Contributor Author

pavolloffay commented Jan 22, 2024

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

@lukastaegert could you please enable the CI again :)

@pavolloffay
Copy link
Contributor Author

zig does not seem to work

Internal Error: powerpc64le-unknown-linux-gnu can not be cross compiled by zig
    at BuildCommand.<anonymous> (/home/runner/work/rollup/rollup/node_modules/@napi-rs/cli/scripts/index.js:11446:27)
    at Generator.next (<anonymous>)
    at /home/runner/work/rollup/rollup/node_modules/@napi-rs/cli/scripts/index.js:3552:69
    at new Promise (<anonymous>)
    at __awaiter$1 (/home/runner/work/rollup/rollup/node_modules/@napi-rs/cli/scripts/index.js:3548:10)
    at BuildCommand.execute (/home/runner/work/rollup/rollup/node_modules/@napi-rs/cli/scripts/index.js:11325:16)
    at BuildCommand.validateAndExecute (/home/runner/work/rollup/rollup/node_modules/@napi-rs/cli/scripts/index.js:[21](https://github.com/rollup/rollup/actions/runs/7617255446/job/20757575020?pr=5350#step:12:22)13:37)
    at /home/runner/work/rollup/rollup/node_modules/@napi-rs/cli/scripts/index.js:3154:53
    at noopCaptureActivator (/home/runner/work/rollup/rollup/node_modules/@napi-rs/cli/scripts/index.js:3380:12)
    at Cli.run (/home/runner/work/rollup/rollup/node_modules/@napi-rs/cli/scripts/index.js:3154:30)

Any help would be appreciated. Now I am stuck again.

@lukastaegert
Copy link
Member

lukastaegert commented Jan 23, 2024

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.

@uweigand
Copy link

Given the success we've had with s390x here #5346, I think the same approach ought to work for ppc64le as well. This means:

  • removing the zig: true line again; and
  • setting the cargo linker environment variable before the npm build call, something like
              export CARGO_TARGET_POWERPC64LE_UNKNOWN_LINUX_GNU_LINKER=powerpc64le-linux-gnu-gcc

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.

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.

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.80%. Comparing base (fffaede) to head (e05cb32).

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.
📢 Have feedback on the report? Share it here.

@Swapnali911
Copy link

Swapnali911 commented Mar 28, 2024

Given the success we've had with s390x here #5346, I think the same approach ought to work for ppc64le as well. This means:

  • removing the zig: true line again; and
  • setting the cargo linker environment variable before the npm build call, something like
              export CARGO_TARGET_POWERPC64LE_UNKNOWN_LINUX_GNU_LINKER=powerpc64le-linux-gnu-gcc

Thanks @uweigand I have tried cross build for ppc64le and it is working as expected
cc : @omajid @tmds @alhad-deshpande @janani66

@lukastaegert lukastaegert added this pull request to the merge queue Mar 28, 2024
Merged via the queue into rollup:master with commit 1217787 Mar 28, 2024
30 checks passed
@uweigand
Copy link

Thanks again, @lukastaegert ! Will this be in the 4.13.2 release then?

Copy link

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

@lukastaegert
Copy link
Member

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.

@uweigand
Copy link

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 },
Copy link
Contributor

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

Copy link

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

PR here: #5460

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?

Copy link
Contributor

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 😅

Choose a reason for hiding this comment

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

I see, thanks!

Copy link
Member

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

@lukastaegert lukastaegert mentioned this pull request Apr 8, 2024
9 tasks
Copy link

This issue has been resolved via #5460 as part of rollup@4.14.2. You can test it via npm install rollup.

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