ci/release: Use LLVM from Rust CI, support macOS and Linux ARM #322
Conversation
vadorovsky
force-pushed
the
binstall
branch
30 times, most recently
from
784db70 to
f38cfb3
Compare
Current libbpf works just fine without it. Furthermore, that package is not available for non x86_64 architectures.
To make the configuration matrix less repetitive, move the toolchain parameters directly under `matrix`, letting GitHub actions produce combinations. Use `include` only to specify the parameters of a job using LLVM from sources. That will let us to add more platforms LLVM installation sources and installation sources with less effort.
It's useful for release jobs as well. Extract it to a separate action. Use `-d` instead of `--max-depth` to make it compatible with the BSD implementation of `du` that's used on macOS. Do not check the APT directory on macOS.
vadorovsky
left a comment
vadorovsky
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 13 files reviewed, 42 unresolved discussions (waiting on @alessandrod, @copilot, and @tamird)
Previously, tamird (Tamir Duberstein) wrote…
Nice!
Done.
Previously, tamird (Tamir Duberstein) wrote…
consider putting these simpler commits ahead of the first commit to reduce rebase churn as we go through this review.
Done
build.rs line 15 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
https://developers.google.com/style/lists#numbered-lettered-bulleted-lists
Done.
build.rs line 194 at r1 (raw file):
Previously, vadorovsky (Michal R) wrote…
Emitting a warning sounds good.
Done.
build.rs line 257 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Yeah, that's definitely simpler! I still think emitting some information when applying first-wins would be useful.
Done.
build.rs line 14 at r11 (raw file):
you could do
w.write_all($bytes.as_ref())in the body so that you can pass string literals rather thanb"..."
Great idea! I indeed can pass string literals.
and
<path>.as_os_str()rather than<path>.as_os_str().as_bytes().
That doesn't work, because OsStr does not implement AsRef<[u8]>. as_bytes is a part of a unix-specific OsStrExt. So I still need to call it explicitly.
build.rs line 19 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
consider calling io::stdout() fewer times since it acquires a lock
Done.
build.rs line 26 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
eh I don't think this is necessary
Done.
build.rs line 102 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
"failed to parse data of static library archive member {} as object file"
Done.
build.rs line 110 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
technically we got an error getting the name, "failed to ... in static library archive member {}"?
Done.
build.rs line 133 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
could avoid the allocations with a Cow!
Given that we also have the case where we have two C++ stdlib files (static LLVM libc++ comes in two files - libc++.a+ and libc++abi.a), I wrote my own enum.
build.rs line 151 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
could avoid the allocations with a Cow!
Done.
build.rs line 185 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
noble but idk if worth it to hardcode lengths
Done.
build.rs line 302 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
needs newline? i think you always want a newline, so you may as well name print_bytes something like
println_bytes
Done.
build.rs line 343 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
let llvm_config = env::split_paths(&paths_os) .find_map(|dir| { let candidate = Path::new(&dir).join("llvm-config"); candidate.try_exists().map(|exists| exists.then_some(candidate)).transpose() }) .transpose() .with_context(|| format!("could not find llvm-config in {var_name}"))?;I think?
Almost. Your llvm_config variable is still an Option, but I can handle it with or_or_else.
Also, your with_context applies the message to an error returned by try_exist, where I think a message like "failed to inspect the file {}" would be more correct. "could not find..." suits more as an error for handling an Option with ok_or_else.
build.rs line 367 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
isn't this just
else? see comment below about avoiding duplication with what's inmain
Done according to the comment below
build.rs line 388 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
i think this logic can be in
link_llvm, something right at the top likelet link_fn = if cfg!(feature = "no-llvm-linking") { if cfg!(feature = "llvm-link-static") { // bail } return; } else if cfg!(feature = "llvm-link-static") { link_llvm_static } else { link_llvm_dynamic };or the logic can stay here and you pass the function into
link_llvm.
With the first idea, we can just make the former link_llvm the main function.
Cargo.toml line 70 at r11 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Let's put llvm-link-static above this one? and then we should say here that we're linking libLLVM's dependencies statically rather than enumerating them. How does that sound?
Done. I also put the version features and rust-* features first.
Cargo.toml line 117 at r12 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Is this the last version that used LLVM 19? Add a comment.
Yes, Rust 1.86.0 is the last version that used LLVM 19.
But now I decided to just drop LLVM 19. Rust 1.87.0 gives us so many nice things, like chained if let and OsStr::display(). I initially added LLVM 19 support, because of Linux distros that are packaging mitmproxy. But given that for now it's just Arch, Gentoo and NixOS and all of them use LLVM >=20, I think it's fine to drop it. 😛
.github/workflows/ci.yml line 49 at r15 (raw file):
To be honest, I don't even understand this comment.
because we care about the apt packages available
We use old Ubuntu because we care about apt packages available? What? 😅
I guess the real reason why we test on older Ubuntu LTS, is that we want to support it, which is a fair goal. If you agree with this explanation, I will rephrase the comment.
.github/workflows/ci.yml line 111 at r15 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
🤔 why only on x86?
There is no gcc-multilib package on arm64.
Anyway, now I removed this block entirely. I don't think we need that package at all - libbpf seems to work just fine without it. And in theory, gcc-multilib should be only needed when building something for 32-bit x86. The fact that the <asm/types.h> headers weren’t available without it sounds pretty sus - it either was never the case, or there was some weird packaging bug.
.github/workflows/ci.yml line 73 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this is broken, shared and dynamic are the same thing. i think you meant to say Apple and Linux musl come with static ones?
Yes, sorry, that's what I meant.
.github/workflows/ci.yml line 87 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
one thing that's a bummer about this representation is that we end up doing a bunch of repeated work e.g. we spin two jobs for x86 linux even though almost all the work done is the same. perhaps instead of this we could just say llvm-from "rust-ci" and then bake the knowledge of which target is shared vs static into the steps below (instead of baking it into the matrix)
Good idea, done.
.github/workflows/ci.yml line 186 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
we probably don't need to get this from the bleeding edge LLVM so we could keep the LLVM APT dance in the LLVM install stanza above.
I removed the step.
.github/workflows/ci.yml line 219 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
interesting, is this because
>> $GITHUB_PATHputs things at the front of PATH?also, can this just be unconditional on linux? we're already installing FileCheck above via apt
I removed this step now. After applying your suggestion about unifying rust-ci-{static,shared} into one, this is not needed - we can just use FileCheck from the GNU tarballs.
.github/workflows/ci.yml line 225 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
s/our GitHub/GitHub Actions/
Done.
.github/workflows/release.yml line 36 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Let's come back to this after we resolve the other thread.
OK, since the ABI incompatibility myth is busted, I'm linking the deps too.
.github/workflows/release.yml line 46 at r20 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
are we gonna need releases for each llvm version we support?
The idea which @alessandrod and me are on board in is producing multiple binaries per supported LLVM version (bpf-linker-20, bpf-linker-21) and the main one (bpf-linker) would autodetect which LLVM version is needed for the inputs, then call the appropriate binary. This way, the main binary would work with different Rust versions.
The downside of having different releases per LLVM version would be a necessity to reinstall when switching toolchains. Currently, rustc just looks for bpf-linker in PATH.
I haven't started any work on this yet, I think I would prefer to land this PR first, then #321, and implement that idea afterwards.
xtask/src/main.rs line 221 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
you shouldn't need this;
Command::ouputtakes&mut selfhttps://doc.rust-lang.org/stable/std/process/struct.Command.html#method.output
sigh, it looks like reviewable completely wrecked your comments in this file :(
Not sure what do you mean. I'm assigning output to the variable, to be able to reuse it in the error message.
xtask/src/main.rs line 252 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
var_os
Done.
xtask/src/main.rs line 256 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
lmao do we need this
also this can be HeaderValue::from_static
https://docs.rs/reqwest/latest/reqwest/header/struct.HeaderValue.html#method.from_static
ditto below
though I think life is maybe simpler when using requestbuilder::json and requestbuilder::bearer_auth
https://docs.rs/reqwest/latest/reqwest/blocking/struct.RequestBuilder.html#method.bearer_auth
If you mean why I have to do into_iter().collect() - the reason is that HeaderMap implements FromIterator, but doesn't have any convenient From implementation. HeadersMap::insert returns an error, that's annoying - the advantage of collecting an iterator is that I don't have to sprinkle errors.
xtask/src/main.rs
Outdated
| let _cmd = rustc_cmd.arg(format!("+{toolchain}")); | ||
| } | ||
| let _cmd = rustc_cmd.args(["--version", "--verbose"]); |
There was a problem hiding this comment.
Actually it's rustc, not clippy:
warning: unused result of type `&mut std::process::Command`
--> xtask/src/main.rs:218:9
|
218 | rustc_cmd.arg(format!("+{toolchain}"));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: requested on the command line with `-W unused-results`
warning: unused result of type `&mut std::process::Command`
--> xtask/src/main.rs:220:5
|
220 | rustc_cmd.args(["--version", "--verbose"]);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
warning: `xtask` (bin "xtask") generated 2 warnings
warning: `xtask` (bin "xtask" test) generated 2 warnings (2 duplicates)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.71s
248db33 to
2bc24ef
Compare
llvm-sys' build.rs[0] uses the output of `llvm-config` binary[1] to figure out where libLLVM libraries live and which additional system libraries to link. There are several problems with that approach. The first one is the fact that the host system has to execute `llvm-config`, which is not always possible. For instance, binaries in LLVM tarballs provided by Rust community for `*-musl` targets are linked dynamically to musl, which makes it impossible to execute on distros with GNU userland. At the same time, libraries in the same tarball are linked statically to musl (and dynamically to libstdc++), which means that it's perfectly fine to use them even in GNU userland. Executing binaries also makes cross-compilation hard. Avoiding these problems by omitting the build.rs of llvm-sys was suggested by upstream[2]. The second problem is that `llvm-config` includes all system libraries that were used for all LLVM subprojects that were built together. Given that the most of package providers build the whole LLVM monorepo once and then split binary artifacts into different packages, there are dependencies we don't need, like libpolly (which gives hard time to our users, see aya-rs#29). The system libraries we need are: C++ standard library and compression libraries (zlib and/or zstd, depends on the build). Furthermore, llvm-sys hard codes the system library paths, making assumptions about different operating systems[3]. We avoid that by asking the C compiler about the library paths, which is more reliable. Last but not least, llvm-sys differentiates between `force-{dynamic,static}` and `prefer-{dynamic,static}` features, giving uncertainity about the linkage. There is also no direct control over how other system libraries are linked. Our build.rs builds everything dynamically by default and honors two features: - `llvm-link-static`, triggering static linkage of libLLVM. - `llvm-deps-link-static`, triggering static linkage of libraries that libLLVM depends on (zlib, zstd). `llvm-deps-link-static` requires `llvm-link-static` - when libLLVM is linked dynamically, static linkage of dependencies makes no sense. Shared libLLVM libraries should come with `DT_NEEDED` referencing all necessary dependencies. What's great is that these entries are limited to C++ stdlib and compression libraries and libpolly is not included. [0] https://gitlab.com/taricorp/llvm-sys.rs/-/blob/e2e64ff03193cd1b548942ac71ec46baa2ad17d8/build.rs [1] https://gitlab.com/taricorp/llvm-sys.rs/-/blob/e2e64ff03193cd1b548942ac71ec46baa2ad17d8/build.rs#L224-231 [2] https://gitlab.com/taricorp/llvm-sys.rs/-/merge_requests/49#note_2130852482 [3] https://gitlab.com/taricorp/llvm-sys.rs/-/blob/e2e64ff03193cd1b548942ac71ec46baa2ad17d8/build.rs#L388-427 Fixes: aya-rs#29
1.90.0 is the latest version that uses LLVM 20. Use it instead of 1.89.0. Always use the latest stable, to be able to catch updates in CI.
Add a subcommand that prints the github.com/rust-lang/rust commit from master that corresponds to an LLVM update for the current Rust toolchain. The goal is to make downloading LLVM artifacts from Rust CI easier.
vadorovsky
force-pushed
the
binstall
branch
2 times, most recently
from
fbacf75 to
43867e9
Compare
vadorovsky
left a comment
vadorovsky
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 13 files reviewed, 42 unresolved discussions (waiting on @alessandrod and @tamird)
Cargo.toml line 86 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Let's discuss in the other thread.
Removed.
.github/workflows/ci.yml line 146 at r16 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this is slightly spooky, does it need a comment?
What's spooky here? This is exactly how it used to be.
.github/workflows/ci.yml line 80 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
leaving a placeholder here because this ABI compat claim might need revisiting after the first commit discussion is resolved
Removed.
.github/workflows/ci.yml line 207 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
use a switch-case here and explicitly blow up on an unexpected value (i think this is checked above but it might one day not be)
Removed the comment and I've managed to remove the libpolly requirement.
.github/workflows/ci.yml line 216 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
does this need a comment?
No, the requirement of libpolly is removed now.
xtask/src/main.rs line 270 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why not search commits? https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28#search-commits
wouldn't that get you there in a single API call instead of two?
The problem is that you have to find the bors' merge commit that bumps LLVM, e.g.:
The commit title is "Auto merge of... [branch-name]". The actual title of the PR - "Update LLVM..." - is embedded in the commit message. There is no search qualifier which allows full-text search of commit messages in /search/commits?q=.
vadorovsky
force-pushed
the
binstall
branch
3 times, most recently
from
7cb7256 to
861434b
Compare
We are planning to use libLLVM from Rust CI to produce static binaries. This change makes sure that tests work with Rust's libLLVM artifacts and libLLVM linked statically (from any source). Rust CI ships only one flavor of libLLVM, shared or static, per target. Linux GNU targets come with shared ones. Apple and Linux musl targets come with dynamic ones. To save disk space, run `cargo hack --feature-powerset` only when shared libraries are used. Otherwise, run cargo with a single feature set.
- Use LLVM from Rust CI instead of LLVM built from source. - Support for macOS and Linux ARM. - Add the disk usage reports. - To make sure that the job does not regress, execute it in a "dry run" mode for each pull request.
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
@tamird reviewed 13 of 13 files at r21, 1 of 1 files at r22, 5 of 5 files at r23, 7 of 7 files at r24, 8 of 8 files at r32, 6 of 6 files at r33, 1 of 1 files at r34, 1 of 1 files at r35, 1 of 1 files at r36, 4 of 4 files at r37, 2 of 2 files at r40, 1 of 1 files at r41, 2 of 2 files at r43, 1 of 1 files at r44, 1 of 1 files at r45, 1 of 1 files at r46, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @vadorovsky)
build.rs line 257 at r1 (raw file):
Previously, vadorovsky (Michal R) wrote…
Done.
this one is not done?
.github/workflows/ci.yml line 49 at r15 (raw file):
Previously, vadorovsky (Michal R) wrote…
To be honest, I don't even understand this comment.
because we care about the apt packages available
We use old Ubuntu because we care about apt packages available? What? 😅
I guess the real reason why we test on older Ubuntu LTS, is that we want to support it, which is a fair goal. If you agree with this explanation, I will rephrase the comment.
We used to care about the specific version of clang that was installed (but not in PATH) see aya-rs/aya@59a1530. I'm not sure if it mattered here in bpf-linker.
.github/workflows/ci.yml line 111 at r15 (raw file):
Previously, vadorovsky (Michal R) wrote…
There is no
gcc-multilibpackage on arm64.Anyway, now I removed this block entirely. I don't think we need that package at all - libbpf seems to work just fine without it. And in theory,
gcc-multilibshould be only needed when building something for 32-bit x86. The fact that the<asm/types.h>headers weren’t available without it sounds pretty sus - it either was never the case, or there was some weird packaging bug.
.github/workflows/ci.yml line 146 at r16 (raw file):
Previously, vadorovsky (Michal R) wrote…
What's spooky here? This is exactly how it used to be.
DYLD_LIBRARY_PATH is new, isn't it?
.github/workflows/ci.yml line 216 at r19 (raw file):
Previously, vadorovsky (Michal R) wrote…
No, the requirement of libpolly is removed now.
I think you need to click on the r19 at the top right corner of this comment box (and others) to properly see what I was commenting on.
In this case it was on LD_LIBRARY_PATH
xtask/src/main.rs line 221 at r17 (raw file):
Previously, vadorovsky (Michal R) wrote…
sigh, it looks like reviewable completely wrecked your comments in this file :(
Not sure what do you mean. I'm assigning
outputto the variable, to be able to reuse it in the error message.
It was a comment on cmd_for_error which seems to be gone now.
xtask/src/main.rs line 256 at r17 (raw file):
Previously, vadorovsky (Michal R) wrote…
If you mean why I have to do
into_iter().collect()- the reason is thatHeaderMapimplementsFromIterator, but doesn't have any convenientFromimplementation.HeadersMap::insertreturns an error, that's annoying - the advantage of collecting an iterator is that I don't have to sprinkle errors.
No this comment was anchored on USER_AGENT.
But see the other comments, maybe RequestBuilder is better here?
xtask/src/main.rs line 270 at r17 (raw file):
Previously, vadorovsky (Michal R) wrote…
The problem is that you have to find the bors' merge commit that bumps LLVM, e.g.:
The commit title is "Auto merge of... [branch-name]". The actual title of the PR - "Update LLVM..." - is embedded in the commit message. There is no search qualifier which allows full-text search of commit messages in
/search/commits?q=.
Ah it turns out the issue is actually that github doesn't index commits for big repos like rust. drat.
.github/workflows/ci.yml line 135 at r41 (raw file):
# [1] https://github.com/llvm/llvm-project/commit/dc1c43d run: | set -euxo pipefail
why this change?
.github/workflows/ci.yml line 310 at r41 (raw file):
# # libLLVM from homebrew requires zstd. run: |
i think we need the set -euxo pipefail any time we have multiline scripts like this
build.rs line 106 at r33 (raw file):
OsStr::from_bytes(first).display().fmt(f)?; } for lib in libs {
easier with .enumerate, no?
build.rs line 260 at r33 (raw file):
false }; let zstd_found = if needs_zstd {
needs_zstd && emit_search_path_if_defined(stdout, "LIBZSTD_PATH")?? same elsewhere
build.rs line 321 at r33 (raw file):
for ld_path in env::split_paths(ld_paths) { let mut found_any = false; if !cxxstdlib_found && let Some(ref mut cxxstdlib_paths) = cxxstdlib_paths {
why if !cxxstdlib_found? i think this is the same as the let
build.rs line 333 at r33 (raw file):
} if needs_zlib && !zlib_found
sam here
build.rs line 345 at r33 (raw file):
} if needs_zstd && !zstd_found
and here
build.rs line 358 at r33 (raw file):
if found_any { write_bytes!( io::stdout(),
stdout
build.rs line 393 at r33 (raw file):
write_bytes!(stdout, ", ", path.as_os_str().as_bytes())?; } write_bytes!(stdout, "\n")?;
ditto, easier with enumerate? also i think you can just use write! for all of it so that you can bake the newline into write_bytes
Code quote:
let mut paths = paths.iter();
if let Some(first) = paths.next() {
write_bytes!(stdout, first.as_os_str().as_bytes())?;
}
for path in paths {
write_bytes!(stdout, ", ", path.as_os_str().as_bytes())?;
}
write_bytes!(stdout, "\n")?;.github/workflows/ci.yml line 118 at r33 (raw file):
sudo apt update sudo apt -y install llvm-${{ matrix.llvm-version }}-dev echo /usr/lib/llvm-${{ matrix.llvm-version }}/bin >> $GITHUB_PATH
leftover?
|
@tamird We have one build failure, which I understand why is happening, but I'm unsure which solution is the least terrible. We are facing this error when trying to link statically LLVM from sources: https://github.com/aya-rs/bpf-linker/actions/runs/19768633299/job/56647522117#step:27:38 The error suggests that the libLLVM was linked to stdlibc++ 12 (dynamically), but now we are trying to link stdlibc++ 11. stdlibc++ is forwards, but not backwards compatible. libLLVM has undefined But the question is - why the heck was our custom LLVM build linked to a newer libstdc++? The answer is - we build LLVM with clang: Lines 111 to 112 in a6069ff that reports the following library directories: The second library directory - The default C compiler, however, is GCC 11: Which reports the following directories:
That said, GCC 12 called explicitly reports its own directories: And therefore, building bpf-linker with Same with So now it looks like we have the following options to solve the problem:
I'm personally leaning towards 1) - starting from a) and falling back to b) if it gets too messy. Any preferences from your side? |
Oof. What about 2) but just do it unconditionally? |
2 participants
Publish statically linked libraries for:
Use libLLVM artifacts from Rust CI. To make sure that the release job does not regress, execute it in a "dry run" mode for each pull request
See individual commits for details.
This change is