Skip to content

Conversation

@paulhauner
Copy link
Member

@paulhauner paulhauner commented Aug 10, 2020

Issue Addressed

NA

Proposed Changes

Adds support for using the cross project to produce cross-compiled binaries using Docker images.

Provides quite clean and simple cross-compiles cause all the complexity is hidden in Dockerfiles. It does require you to be in the docker group though.

Details

  • Adds shortcut commands to Makefile
  • Ensures reqwest and discv5 use vendored openssl libs (i.e., static not shared).
  • Switches to a commit of blst that has a renamed C function to avoid a collision with openssl (upstream issue: Symbol collision with openssl supranational/blst#21).
  • Updates ring to the latest satisfiable version, since an earlier version was causing issues with cross.
  • Off-topic, but adds extra message about Windows support as suggested by Discord user.

Additional Info

  • Blocked on [Merged by Bors] - Upgrade logs #1495
  • There are no tests in CI for this yet for a few reasons:
    • I'm hesitant to add more long-running tasks.
    • Short-term bitrot should be avoided since we'll use it each release.
    • In the long term I think it would be good to automate binary creation on a release.
  • I observed the binaries increase in size from 50mb to 52mb after these changes.

Squashed commit of the following:

commit fe3ecc0
Author: Age Manning <Age@AgeManning.com>
Date:   Mon Aug 10 13:59:29 2020 +1000

    Report port overlapping and failure to bind to user

commit d3dc6a9
Author: Age Manning <Age@AgeManning.com>
Date:   Mon Aug 10 12:01:27 2020 +1000

    Upgrade address conflict logs
@paulhauner paulhauner added work-in-progress PR is a work-in-progress blocked labels Aug 10, 2020
@paulhauner paulhauner added low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Aug 10, 2020
@paulhauner paulhauner marked this pull request as ready for review August 10, 2020 23:18
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Just a few typos that need fixing before merge. We can address the remaining portability issues separately.

> Note: if you're not familiar with Rust or you'd like more detailed instructions, see our [installation guide](./installation.md).
> Notes:
> - If you're not familiar with Rust or you'd like more detailed instructions, see our [installation guide](./installation.md).
> - Windows is presently only supported via [WSL](target/aarch64-unknown-linux-gnu).
Copy link
Member

Choose a reason for hiding this comment

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

typo in link target, should be WSL not aarch64

@@ -0,0 +1,41 @@
# Cross-compiling

Lighthouse supports cross-compiling, allowing users to run a binary one
Copy link
Member

Choose a reason for hiding this comment

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

typo: "on one platform"?

for most users).
- `build-x86_64-portable`: builds a version x86_64 processors which avoids
using some modern CPU instructions that might cause an "illegal
instruction" error on older CPUs.
Copy link
Member

Choose a reason for hiding this comment

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

There's still an issue with pre-SSSE3 CPUs. I've opened #1504 to track it


The `Makefile` in the project contains four targets for cross-compiling:

- `build-x86_64`: builds an optimized version for x86_64 processors (suitable
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that this produces a portable binary, but it seems to (I ran the v0.2.2 release on my laptop just fine). The binary must still be compiled with -march=native inside the Docker image, which would tailor it to the host machine, so I'm not sure if we just got lucky with the choice of host. I've opened #1505 to track this.

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Aug 11, 2020
@paulhauner
Copy link
Member Author

Thanks @michaelsproul, I believe I added all the changes required for now.

@paulhauner paulhauner added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 11, 2020
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

bors r+

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 11, 2020
bors bot pushed a commit that referenced this pull request Aug 11, 2020
## Issue Addressed

NA

## Proposed Changes

Adds support for using the [`cross`](https://github.com/rust-embedded/cross) project to produce cross-compiled binaries using Docker images.

Provides quite clean and simple cross-compiles cause all the complexity is hidden in Dockerfiles. It does require you to be in the `docker` group though.

## Details

- Adds shortcut commands to `Makefile`
- Ensures `reqwest` and `discv5` use vendored openssl libs (i.e., static not shared).
- Switches to a [commit](sigp/blst@284f705) of blst that has a renamed C function to avoid a collision with openssl (upstream issue: supranational/blst#21).
- Updates `ring` to the latest satisfiable version, since an earlier version was causing issues with `cross`.
- Off-topic, but adds extra message about Windows support as suggested by Discord user.

## Additional Info

- ~~Blocked on #1495~~
- There are no tests in CI for this yet for a few reasons:
  - I'm hesitant to add more long-running tasks.
  - Short-term bitrot should be avoided since we'll use it each release.
  - In the long term I think it would be good to automate binary creation on a release.
- I observed the binaries increase in size from 50mb to 52mb after these changes.
@bors
Copy link

bors bot commented Aug 11, 2020

@bors bors bot changed the title Cross-compile to vendored x86_84, aarch64 (Raspberry Pi 4) [Merged by Bors] - Cross-compile to vendored x86_84, aarch64 (Raspberry Pi 4) Aug 11, 2020
@bors bors bot closed this Aug 11, 2020
paulhauner added a commit that referenced this pull request Aug 12, 2020
## Issue Addressed

NA

## Proposed Changes

Adds support for using the [`cross`](https://github.com/rust-embedded/cross) project to produce cross-compiled binaries using Docker images.

Provides quite clean and simple cross-compiles cause all the complexity is hidden in Dockerfiles. It does require you to be in the `docker` group though.

## Details

- Adds shortcut commands to `Makefile`
- Ensures `reqwest` and `discv5` use vendored openssl libs (i.e., static not shared).
- Switches to a [commit](sigp/blst@284f705) of blst that has a renamed C function to avoid a collision with openssl (upstream issue: supranational/blst#21).
- Updates `ring` to the latest satisfiable version, since an earlier version was causing issues with `cross`.
- Off-topic, but adds extra message about Windows support as suggested by Discord user.

## Additional Info

- ~~Blocked on #1495~~
- There are no tests in CI for this yet for a few reasons:
  - I'm hesitant to add more long-running tasks.
  - Short-term bitrot should be avoided since we'll use it each release.
  - In the long term I think it would be good to automate binary creation on a release.
- I observed the binaries increase in size from 50mb to 52mb after these changes.
@paulhauner paulhauner deleted the cross-compile branch October 5, 2020 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-merge This PR is ready to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants