Plume/Plume
9
18
Fork 24
Code Issues 177 Pull requests 8 Releases 8 Wiki Activity

Plume front arm support #402

Merged
mcrosson merged 7 commits from plume-front-arm-support into master 2018-12-31 17:46:21 +00:00
Conversation 4 Commits 7 Files changed 3 +49 -2
mcrosson commented 2018-12-31 15:26:02 +00:00 (Migrated from github.com)
Copy link

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 2018-12-31 15:59:53 +00:00 (Migrated from github.com)
Copy link

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 2018-12-31 16:18:20 +00:00
Owner
Copy link

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 2018-12-31 16:27:37 +00:00
script/plume-front.sh
trinity-1686a commented 2018-12-31 16:27:22 +00:00
Owner
Copy link

(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 2018-12-31 16:28:07 +00:00
script/plume-front.sh
mcrosson (Migrated from github.com) commented 2018-12-31 16:28:07 +00:00
Copy link

Good point, I'll update the script.

Good point, I'll update the script.
trinity-1686a requested changes 2018-12-31 17:02:29 +00:00
trinity-1686a left a comment
Owner
Copy link

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

>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 2018-12-31 16:53:55 +00:00
Owner
Copy link

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 2018-12-31 17:06:15 +00:00
Dockerfile
mcrosson (Migrated from github.com) commented 2018-12-31 17:06:15 +00:00
Copy link

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 2018-12-31 17:20:22 +00:00
trinity-1686a reviewed 2018-12-31 17:29:10 +00:00
Dockerfile
trinity-1686a commented 2018-12-31 17:29:10 +00:00
Owner
Copy link

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 2018-12-31 17:35:18 +00:00
Dockerfile
mcrosson (Migrated from github.com) commented 2018-12-31 17:35:18 +00:00
Copy link

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 2019-03-29 06:03:25 +00:00 (Migrated from github.com)
Copy link

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 2019-03-29 15:22:41 +00:00 (Migrated from github.com)
Copy link

@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.
Sign in to join this conversation.
Reviewers
No reviewers
trinity-1686a
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 labels
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
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Plume/Plume#402
No description provided.
Delete branch "plume-front-arm-support"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?