Plume
/
Plume
7
9
Fork 8
Code Issues 131 Pull Requests 13 Releases 4 Wiki Activity
Labels Milestones
New Pull Request

#402 Plume front arm support

Merged
mcrosson merged 7 commits from plume-front-arm-support into master 1 year ago
Conversation 4 Commits 7 Files Changed 3
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
script/plume-front.sh
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
script/plume-front.sh
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

Dockerfile
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
Dockerfile
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
Dockerfile
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
Dockerfile
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.
Reviewers
Request review
No reviewers
 trinity-1686a
Labels
Apply labels
Clear labels
A: API
Related to the REST API A: Backend
Code running on the server A: Federation
Stuff related to Federation A: Front-End
Related to the front-end A: I18N
Translations, and related code A: Meta
More about project management or code than the project itself A: Security Build
The building, or installation process of Plume C: Bug
Something isn't working C: Discussion
We need to talk C: Enhancement
New feature or request C: Feature
This is a new feature Compatibility
Compatibility with different browsers, readers and OS Dependency
Related to an external package that Plume uses Design
UI/UX related issues and PRs Documentation Good first issue
Good for newcomers Help welcome
Extra attention is needed Mobile
Issues affecting only mobile UX Rendering
How elements're rendered out for the end user S: Blocked
Something else needs to be fixed first S: Duplicate
This issue or pull request already exists S: Incomplete
This PR is not complete yet S: Instance specific
Issues concern a limited number of instances S: Invalid
This doesn't seem right S: Needs Voting/Discussion
Need to be discussed by the community (on Loomio) S: Ready for review
This PR is ready to be reviewed Suggestion
Proposed ideas worth considering S: Voted on Loomio
This is issue has been created after a vote on Loomio S: Wontfix
This will not be worked on
No Label
A: API
A: Backend
A: Federation
A: Front-End
A: I18N
A: Meta
A: Security
Build
C: Bug
C: Discussion
C: Enhancement
C: Feature
Compatibility
Dependency
Design
Documentation
Good first issue
Help welcome
Mobile
Rendering
S: Blocked
S: Duplicate
S: Incomplete
S: Instance specific
S: Invalid
S: Needs Voting/Discussion
S: Ready for review
Suggestion
S: Voted on Loomio
S: Wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'plume-front-arm-support'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes