#402 Plume front arm support

Merged
mcrosson merged 7 commits from plume-front-arm-support into master 1 year ago
mcrosson commented 1 year ago (Migrated from github.com)

This PR addresses #394 and #400

Changes

  • Update Dockerfile to add necessary lld dependency on arm platforms via a helper script
  • Run the build/install of plume-front with the necessary linker=lld option on arm platforms via a helper script
  • Add rust-toolchain to Docker container along with Cargo files to ensure the right rust toolchain is selected for later steps

Builds Tested On

  • x86-64
  • arm64 (aarch64)
  • arm32 (armv7l)

Noteworthy items

  • The rust wasm packages do NOT include rustc-lld for arm
  • The lld binaries published by llvm.org will NOT build plume-front with linker=lld rust args
  • lld built from sources (what this patch does) on arm DOES build plume-front with linker=lld rust args
This PR addresses #394 and #400 Changes - Update Dockerfile to add necessary lld dependency on arm platforms via a helper script - Run the build/install of plume-front with the necessary linker=lld option on arm platforms via a helper script - Add rust-toolchain to Docker container along with Cargo files to ensure the right rust toolchain is selected for later steps Builds Tested On - x86-64 - arm64 (aarch64) - arm32 (armv7l) Noteworthy items - The rust wasm packages do NOT include rustc-lld for arm - The lld binaries published by llvm.org will NOT build plume-front with linker=lld rust args - lld built from sources (what this patch does) on arm DOES build plume-front with linker=lld rust args
codecov[bot] commented 1 year ago (Migrated from github.com)
Owner

Codecov Report

Merging #402 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #402   +/-   ##
=======================================
  Coverage   27.88%   27.88%           
=======================================
  Files          63       63           
  Lines        7252     7252           
=======================================
  Hits         2022     2022           
  Misses       5230     5230
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/402?src=pr&el=h1) Report > Merging [#402](https://codecov.io/gh/Plume-org/Plume/pull/402?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/2896eb1705a766cc98935d64d090932aa617a0b1?src=pr&el=desc) will **not change** coverage. > The diff coverage is `n/a`. ```diff @@ Coverage Diff @@ ## master #402 +/- ## ======================================= Coverage 27.88% 27.88% ======================================= Files 63 63 Lines 7252 7252 ======================================= Hits 2022 2022 Misses 5230 5230 ```
trinity-1686a commented 1 year ago
Owner

I’d prefer plume-front.sh and wasm-deps.sh to be put in the script folder, else it seems all good

I'd prefer `plume-front.sh` and `wasm-deps.sh` to be put in the `script` folder, else it seems all good
trinity-1686a reviewed 1 year ago
trinity-1686a commented 1 year ago

(you don’t actually need to save PATH, it’s an environment variable as any other, a modification is only applied to this process and its child, not its parents)

(you don't actually need to save PATH, it's an environment variable as any other, a modification is only applied to this process and its child, not its parents)
mcrosson (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

Good point, I’ll update the script.

Good point, I'll update the script.
trinity-1686a requested changes 1 year ago
trinity-1686a left a comment

Builds Tested On

  • x86-64
  • arm64 (aarch64)
  • arm32 (armv7l)

Are you sure you don’t have uncommitted changes? Compilation failed for me with x86-64, with the issue about scoping lint being experimental

trinity-1686a commented 1 year ago

Compilation failed because diesel_cli compilation depends on nightly and stable is used by default

 COPY Cargo.toml Cargo.lock rust-toolchain ./
Compilation failed because diesel_cli compilation depends on nightly and stable is used by default ```suggestion COPY Cargo.toml Cargo.lock rust-toolchain ./ ```
mcrosson (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

I have #400 open for that ; I’ll update per your changes above and include #400 in the main PR comments too.

I have #400 open for that ; I'll update per your changes above and include #400 in the main PR comments too.
trinity-1686a approved these changes 1 year ago
trinity-1686a reviewed 1 year ago
trinity-1686a commented 1 year ago

ho sorry, I through it was in the same issue

ho sorry, I through it was in the same issue
mcrosson (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

No worries, this trick solved something that was bugging me and I’m happy to take care of two tickets with a single PR.

No worries, this trick solved something that was bugging me and I'm happy to take care of two tickets with a single PR.
vielmetti commented 1 year ago (Migrated from github.com)
Owner

The lld binaries published by llvm.org will NOT build plume-front with linker=lld rust args

@mcrosson do you recall what’s missing with the llvm distributed lld binaries? I wonder if that’s something that might be fixed upstream.

> The lld binaries published by llvm.org will NOT build plume-front with linker=lld rust args @mcrosson do you recall what's missing with the llvm distributed lld binaries? I wonder if that's something that might be fixed upstream.
mcrosson commented 1 year ago (Migrated from github.com)
Owner

@vielmetti IIRC the Rust packaging does not include arm32 or 64 as tier 1 for lld-wasm. I think it’s on the WASM side? My memory is mildly hazy, there were a lot of tickets that I read and I’m still not familiar with the rust ecosystem.

@vielmetti IIRC the Rust packaging does not include arm32 or 64 as tier 1 for lld-wasm. I think it's on the WASM side? My memory is mildly hazy, there were a lot of tickets that I read and I'm still not familiar with the rust ecosystem.

Reviewers

trinity-1686a approved these changes 1 year ago
The pull request has been merged as dfa89e227a.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.