-
Notifications
You must be signed in to change notification settings - Fork 47
ci: Build statically linked bpf-linker in CI #215
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
base: main
Are you sure you want to change the base?
Conversation
c362c1e
to
42e16f3
Compare
7a507e8
to
72afe24
Compare
fbb5e3a
to
2b3fc8b
Compare
35354c9
to
5615015
Compare
ae099ec
to
d8d1002
Compare
05f0283
to
7d0e57e
Compare
e129b7e
to
ded5cdd
Compare
@tamird The last CI run was fully green and this one should be too (I just removed action-tmate, lol). Thanks for all the icedragon reviews! It looks like now it's able to do its job properly. I'm not happy from the amount of if statements in the action yaml. I'm wondering if I should just split the musl jobs into completely different job, which would result in a bit of repetition, but some of the ugliest if statements would go away. I'm also thinking if I should write some icedragon github action for running different commands, so we could get rid of some boilerplate. |
I will mark it as ready to review once all icedragon PRs are merged and I do a release of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to address existing comments
Reviewed 2 of 23 files at r10, all commit messages.
Reviewable status: 2 of 25 files reviewed, 16 unresolved discussions (waiting on @vadorovsky)
build-llvm.sh
line 1 at r11 (raw file):
#!/usr/bin/env sh
let's python it. i'm scared of bash over 10 lines
3bf92d8
to
8457873
Compare
Build LLVM and bpf-linker with statically linked libLLVM for multiple architectures. Include the resulting build artifacts on the release pages, so they can be installed with `cargo binstall`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 25 files reviewed, 16 unresolved discussions (waiting on @tamird)
build-llvm.sh
line 1 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
let's python it. i'm scared of bash over 10 lines
Hmm... I can try, but I can image such Python script ending up being a bunch of subprocess.run
/subprocess.Popen
calls. Would you be open to an xtask with xshell as an alternative? In the meantime, I will play with both Python and xtask and see what ends up being less ugly in my opinion.
Cargo.toml
line 66 at r9 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
what's going on here? the same information is at the top of this file
nuked
Cargo.toml
line 72 at r9 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
more repetition
nuked
.github/workflows/ci.yml
line 67 at r7 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Yeah mixing libs c would be surprising if it worked.
Yeah, it doesn't, I dropped that approach.
.github/workflows/ci.yml
line 59 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
This should be anchored on an instance of
ubuntu-22.04
Everwthing is switched to ubuntu-22.04 now
.github/workflows/ci.yml
line 61 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Can we add a comment about why we can't cross-compile for these apple targets?
It's not that we can't, but we don't have to - macos-13 is Intel-based and macos-14 is ARM-based.
I still want to cross-compile on Linux, even though arm64 Linux runners are a thing nowadays, because of musl builds - icedragon working on arm64 hosts. 🫠 I want to make it possible, eventually, but let's not make it a blocked for bpf-linker binstalls. I want to get this done already.
Also, I might add RISC-V Linux targets as well at some point, so having the infra for cross-compiling to Linux targets in general is helpful.
Added comments about that.
.github/workflows/ci.yml
line 101 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Yeah, I think removing from .cargo/config.toml is sensible.
Done. The whole dance with figuring out which qemu emulator to use is done in icedragon.
.github/workflows/ci.yml
line 193 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
don't we need/want no-llvm-linking here too?
We do and it's there now.
.github/workflows/llvm.yml
line 13 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why does this need to run on alpine?
It's not anymore, I'm using Gentoo/icedragon for musl builds now
.github/workflows/llvm.yml
line 42 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
We can get all the way to the full SHA faster and more simply with
yq
:$ curl -sfSL https://static.rust-lang.org/dist/channel-rust-nightly.toml | yq -p toml '.pkg.rustc.git_commit_hash' 636d7ff91b9847d6d43c7bbe023568828f6e3246
Achtschually 🤓 I'm thinking about adding an xtask command for that. I think it's going to be convenient to resolve the LLVM commit for local development as well.
Python is not a good fit for that, since it can't parse yaml without additional pip packages.
.github/workflows/llvm.yml
line 101 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
i think this TODO can go? unclear why we need the latest LLVM, we just need something reasonably recent. In fact we might not need this at all, there's already a clang on mac runners. Maybe we just need ninja.
We need ninja
and lld
, seems like the rest can go away. Why lld? The default linker on macOS is ld64
and lld is not shipped in the base system.
In theory, I could make the build work with ld64, but then I would need to maintain different set of CMake arguments for macOS, so I don't think it's worth it.
.github/workflows/llvm.yml
line 114 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
🤔 I think we no longer want this, we're building from source
We are just building from source here, so I guess this comment is addressed.
.github/workflows/llvm.yml
line 154 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
👍
I ended up removing the dynamic libs. I managed to save some space by doing:
cmake --build build \
--target install-llvm-config \
--target install-llvm-libraries
Which tells cmake to build only libary and llvm-comfig targets. All other binaries, which we previously used to ship in the cashe, are skipped.
see the build-llvm.sh script.
.github/workflows/llvm.yml
line 159 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
we can probably remove?
Whatever was there after build, is removed, so I guess that's addressed
.github/workflows/release.yml
line 12 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Fair. Thanks for trying -- consider leaving a short comment.
I consider this addressed - I moved the injection of cross-specific CMake flags to icedragon and they are documented there. Our scripts and actions don't inject any special per-target flags anymore apart from simply specifying the target.
tests/tests.rs
line 96 at r8 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Was done in #237
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 18 of 23 files at r10, 2 of 5 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: 23 of 25 files reviewed, 28 unresolved discussions (waiting on @vadorovsky)
build-llvm.sh
line 1 at r11 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
Hmm... I can try, but I can image such Python script ending up being a bunch of
subprocess.run
/subprocess.Popen
calls. Would you be open to an xtask with xshell as an alternative? In the meantime, I will play with both Python and xtask and see what ends up being less ugly in my opinion.
i would really prefer no xshell, but xtask seems fine
.github/workflows/llvm.yml
line 42 at r1 (raw file):
Previously, vadorovsky (Michal Rostecki) wrote…
Achtschually 🤓 I'm thinking about adding an xtask command for that. I think it's going to be convenient to resolve the LLVM commit for local development as well.
Python is not a good fit for that, since it can't parse yaml without additional pip packages.
Are you planning to address this?
.github/workflows/llvm.yml
line 35 at r12 (raw file):
# # * icedragon does not support running on arm64 hosts. # * We might add RISC-V targets, so having the infrastructure for
you already added risc-v in release.yml, don't you need it here?
.github/workflows/llvm.yml
line 36 at r12 (raw file):
# * icedragon does not support running on arm64 hosts. # * We might add RISC-V targets, so having the infrastructure for # Linux cross builds is benefiial.
beneficial
.github/workflows/llvm.yml
line 98 at r12 (raw file):
run: | set -euxo pipefail # brew install cmake lld llvm ninja
remove?
.github/workflows/llvm.yml
line 100 at r12 (raw file):
# brew install cmake lld llvm ninja brew install lld ninja # echo $(brew --prefix)/opt/llvm/bin >> $GITHUB_PATH
remove?
.github/workflows/llvm.yml
line 105 at r12 (raw file):
if: steps.cache-llvm.outputs.cache-hit != 'true' run: | ./build-llvm.sh \
${{ github.workspace }}/build-llvm.sh?
.github/workflows/release.yml
line 36 at r12 (raw file):
with: path: llvm-install key: ${{ needs.llvm.outputs[format('cache-key-{0}', matrix.target-llvm)] }}
same question
.github/workflows/release.yml
line 59 at r12 (raw file):
--release - uses: svenstaro/upload-release-action@v2
how about https://github.com/taiki-e/upload-rust-binary-action? that guy makes good stuff
.github/workflows/llvm.yml
line 83 at r12 (raw file):
# # Since the LLVM build creates a bunch of symlinks (and this setting does not turn those # into symlinks-to-symlinks), use absolute symlinks so we can distinguish the two cases.
why did we lose all this?
Code quote:
# Create symlinks rather than copies to conserve disk space. At the time of this writing,
# GitHub-hosted runners have 14GB of SSD space
# (https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources).
#
# Since the LLVM build creates a bunch of symlinks (and this setting does not turn those
# into symlinks-to-symlinks), use absolute symlinks so we can distinguish the two cases.
.github/workflows/ci.yml
line 59 at r12 (raw file):
- beta - nightly # We don't use ubuntu-latest because we care about the apt packages available.
this is still anchored in the wrong place
.github/workflows/ci.yml
line 86 at r12 (raw file):
- uses: actions/checkout@v4 - name: Free disk space
i hate that we have to do this
.github/workflows/ci.yml
line 155 at r12 (raw file):
with: path: llvm-install key: ${{ needs.llvm.outputs[format('cache-key-{0}', matrix.target.target)] }}
what happened here?
.github/workflows/ci.yml
line 191 at r12 (raw file):
if: matrix.target.build-type == 'cross' env: BPF_LINKER_LLVM_PREFIX: "/llvm-install"
this just happens to also be the path in the github workspace. can you please choose something that isn't the same? this is misleading. i.e. the path in the contain should be not the same as the path outside the container.
.github/workflows/ci.yml
line 221 at r12 (raw file):
- name: Test # We can run tests only for the x86_64-unknown-linux-musl cross target.
say why, not what
.github/workflows/ci.yml
line 241 at r12 (raw file):
- name: Install if: matrix.rust == 'nightly' && runner.os == 'Linux' && matrix.target.build-type == 'native' && startsWith(matrix.target.target, 'x86_64')
why?
.github/workflows/ci.yml
line 246 at r12 (raw file):
- name: Install if: matrix.rust == 'nightly' && runner.os == 'Linux' && matrix.target.build-type == 'cross' && startsWith(matrix.target.target, 'x86_64')
why?
README.md
line 71 at r12 (raw file):
Aya community hosts its own fork of LLVM, which usually is no different from the one hosted byi the Rust community, but occasionaly might contain our own
s/byi/by/
can we avoid the word "community" for both aya and rust? these are not community-hosted, they are project-hosted.
README.md
line 74 at r12 (raw file):
patches. Linux builds (of both LLVM and bpf-linker) are using the icedragon tool, which
s/are using/use
README.md
line 75 at r12 (raw file):
Linux builds (of both LLVM and bpf-linker) are using the icedragon tool, which provides a containeried environment with all necessary dependencies.
containerized
README.md
line 100 at r12 (raw file):
For Linux targets, it supports only musl libc (
*-*-linux-musl
targets).
this is a bit misleading, right? yes, it supports musl targets, but the resulting binary can run anywhere because static linking
README.md
line 103 at r12 (raw file):
```sh cargo install bpf-linker --no-default-features
what's this?
build.rs
line 9 at r12 (raw file):
fn main() { // Execute the build script only when custom LLVM path is provided. let llvm_prefix = match env::var("BPF_LINKER_LLVM_PREFIX") {
var_os
build.rs
line 16 at r12 (raw file):
let target = env::var("TARGET").unwrap(); let target = Triple::from_str(&target).unwrap();
let Triple { ... } = ...
build.rs
line 20 at r12 (raw file):
// Use the expected sysroot directories. Otherwise, fall back to the // standard ones. match (
match on os and env just once? they are all the same
build.rs
line 54 at r12 (raw file):
println!("cargo::rustc-link-lib=static=zstd"); // Link libLLVM using the artifacts from the provided prefix.
this comment is a bit strange - maybe better is to explain why we're linking everything in there?
build.rs
line 60 at r12 (raw file):
let entry = entry.expect("failed to retrieve the file in the LLVM build directory"); let path = entry.path(); if path.is_file() && path.extension().and_then(|ext| ext.to_str()) == Some("a") {
why are we operating on strings
build.rs
line 61 at r12 (raw file):
let path = entry.path(); if path.is_file() && path.extension().and_then(|ext| ext.to_str()) == Some("a") { let libname = path.file_name().unwrap().to_string_lossy();
please no lossy
Build LLVM and bpf-linker with statically linked libLLVM for multiple architectures. Include the resulting build artifacts on the release pages, so they can be installed with
cargo binstall
This change is