integration-test: Set rust-lld as a linker only on macOS
#908
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
vadorovsky
force-pushed
the
integration-tests-linker
branch
from
3c4f45c to
b3842e3
Compare
vadorovsky
changed the title
vadorovsky
force-pushed
the
integration-tests-linker
branch
3 times, most recently
from
c3a1ecf to
0107a99
Compare
|
Is this no longer necessary? |
I would say it's still necessary. I'm still facing the same issue when I'm building integration tests on any musl-based system. I asked Rust community about that here rust-lang/rust#130062 and the conclusion is basically: "you're supposed to use the C compiler driver as the linker, as it knows where to find these libraries" It's still a mystery for me why Let me rebase it. The red builds are still the old ones from September, unrelated to the PR itself. |
|
On a side note, Rust nightly is currently using rust-lld by default (but still through the system C compiler which figures out the paths). If they decide to ship that in any stable version, we won't have to touch any RUSTFLAGS here anymore. |
vadorovsky
force-pushed
the
integration-tests-linker
branch
from
0107a99 to
c0a7b7f
Compare
|
Ubuntu fails now because of old clang. I also noticed just now that we are entering the Taking a step back, I'm starting to doubt whether we should mangle with the linkers and linker flags at all in this place. Our default cargo configuration specifies the cross gcc wrappers as linkers, which should be good enough for Ubuntu/Debian/Fedora users even for cross-compilation scenarios. People wanting to link with clang+lld could just define their own RUSTFLAGS (at least I'm fine with doing that,). I will give it a try now, but if not touching the flags is going to work both on Ubuntu and macOS, then I think that would be a solution. |
That doesn't sound right. Are you sure?
How is |
Nevermind, that's not true. What doesn't work is building/running VM tests (on musl/Linux host). In that case After the following change: diff --git a/xtask/src/run.rs b/xtask/src/run.rs
index 8bba7a5..b6e4b2a 100644
--- a/xtask/src/run.rs
+++ b/xtask/src/run.rs
@@ -59,8 +59,7 @@ where
let mut cmd = Command::new("cargo");
cmd.args(["build", "--message-format=json"]);
if let Some(target) = target {
- let config = format!("target.{target}.linker = \"rust-lld\"");
- cmd.args(["--target", target, "--config", &config]);
+ cmd.args(["--target", target]);
}
f(&mut cmd);It works. At least for the x86_64 scenario, where my default C toolchain is picked up. For cross scenarios, I would need to set up RUSTFLAGS manually to pick up clang+lld, but it's fine. I just don't want xtask automatically switching to rust-lld in that case. I also checked whether that change works on Ubuntu and it also works fine. Using C compiler as linker indeed looks like the best approach for Linux.
I see. I guess that I tried whether the combination of So I guess that we could apply |
vadorovsky
force-pushed
the
integration-tests-linker
branch
2 times, most recently
from
ce23f2e to
52045f7
Compare
vadorovsky
changed the title
rust-lld as a linker only on macOS
vadorovsky
force-pushed
the
integration-tests-linker
branch
from
52045f7 to
89098e3
Compare
|
Thanks for the detail. Let's give it a shot. Can we add a CI job that verifies that everything works as expected? |
vadorovsky
force-pushed
the
integration-tests-linker
branch
5 times, most recently
from
8175be3 to
ae94156
Compare
tamird
force-pushed
the
integration-tests-linker
branch
from
ae94156 to
868d1f5
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @vadorovsky)
.github/workflows/ci.yml line 192 at r2 (raw file):
steps: - name: Log in to GHCR run: echo "${{ secrets.GITHUB_TOKEN }}" | docker login ghcr.io -u ${{ github.actor }} --password-stdin
is there a reason we can't use github actions' "native" syntax for docker actions? https://docs.github.com/en/actions/sharing-automations/creating-actions/creating-a-docker-container-action#creating-an-action-metadata-file
.github/workflows/ci.yml line 274 at r2 (raw file):
- name: Install prerequisites if: runner.os == 'Linux' && contains(matrix.container, 'alpine') # Use clang for building the C eBPF programs for integration tests.
can we document each dep please as we do above?
xtask/src/run.rs line 63 at r1 (raw file):
cmd.args(["--target", target]); // Always use rust-lld on macOS hosts. See // https://github.com/aya-rs/aya/pull/908#issuecomment-2402813711
let's not do this. please write a sensible explanation here; don't make me go to a (mutable) comment in github
The recommendation (coming from rust-lang/rust#130062) for Linux hosts is using C compiler driver as a linker, which is able to find system-wide libraries. Using linker binaries directly in `-C linker` (e.g. `-C linker=rust-lld`) often results in errors like: ``` cargo:warning=error: linking with `rust-lld` failed: exit status: 1ger, ppv-lite86, libc... cargo:warning= | cargo:warning= = note: LC_ALL="C" PATH="/home/vadorovsky/.rustup/toolchains/stable-x86_64-un cargo:warning= = note: rust-lld: error: unable to find library -lgcc_s cargo:warning= rust-lld: error: unable to find library -lc cargo:warning= cargo:warning= cargo:warning= cargo:warning=error: aborting due to 1 previous error ``` Not touching the linker settings is usually the best approach for Linux systems. Native builds pick up the default C toolchain. Cross builds default to GCC cross wrapper, but that's easy to supress with clang and lld using RUSTFLAGS. However, `-C linker=rust-lld` still works the best on macOS, where Rust toolchains come with libc and runtime library and there is no need to link any system libraries. Keep setting it only for macOS. Fixes aya-rs#907
vadorovsky
force-pushed
the
integration-tests-linker
branch
2 times, most recently
from
ec3ab30 to
c37355e
Compare
vadorovsky
force-pushed
the
integration-tests-linker
branch
18 times, most recently
from
26f5b24 to
ae8eece
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @tamird)
.github/workflows/ci.yml line 192 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
is there a reason we can't use github actions' "native" syntax for docker actions? https://docs.github.com/en/actions/sharing-automations/creating-actions/creating-a-docker-container-action#creating-an-action-metadata-file
I removed the custom image all together. I left a comment, but tl;dr - runners don't support running containers with regular users, because of host mounts owned by root.
.github/workflows/ci.yml line 274 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we document each dep please as we do above?
done
xtask/src/run.rs line 63 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
let's not do this. please write a sensible explanation here; don't make me go to a (mutable) comment in github
fair, what about now?
vadorovsky
force-pushed
the
integration-tests-linker
branch
from
c7a306b to
213fb75
Compare
It's hard to predict what's the PID of the first process in a container. Use this assertion only on non-containerized systems.
This way we are making sure that the integration tests infra doesn't regress on musl environments.
vadorovsky
force-pushed
the
integration-tests-linker
branch
from
213fb75 to
9e1d8b4
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5, 2 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @vadorovsky)
xtask/src/run.rs line 76 at r5 (raw file):
// linker (like ldd or mold) can be done with `-C link-arg=-fuse-ld=`. // // However, the same doesn't hold true for macOS. Cross toolchains for
i'm not i understand how this works. if we were compiling for linux-gnu instead of linux-musl i think we'd still have a problem, no?
in other words i think this is a property of musl vs gcc rather than the host being mac vs linux, but i haven't tested it. how sure are you?
.github/workflows/ci.yml line 209 at r9 (raw file):
container: image: docker.io/alpine:3.20 options: --privileged -v /sys/fs/bpf:/sys/fs/bpf -v /sys/kernel:/sys/kernel
can you use the volumes attribute?
volumes:
- my_docker_volume:/volume_mount.github/workflows/ci.yml line 230 at r9 (raw file):
# git is needed for the `checkout` action. # # libstdc++ is a dependency of llvm-sys, which is a dependency of
nit: libstdc++-dev, no?
.github/workflows/ci.yml line 285 at r9 (raw file):
# fatal: detected dubious ownership in repository at '/__w/aya/aya' # # Which makes a lot of sense, not running regular git commands as root is
i think this isn't about running git as root, it's about git noticing that you're in a repo that isn't owned by you
.github/workflows/ci.yml line 287 at r9 (raw file):
# Which makes a lot of sense, not running regular git commands as root is # a good thing. However, using a container image with a regular user # results in permission errors thrown by the runner binary.[0] It's most
this should be on the --privileged option way up on the container settings
could we just not run the BPF iterator tests if we notice we're in a container? That would also allow you to ditch the previous commit but the big win is not running as root
.github/workflows/ci.yml line 295 at r9 (raw file):
- name: Mark the directory as safe for git if: matrix.container != '' run: git config --global --add safe.directory /__w/aya/aya
can we avoid using __w? isn't there a github root thing we can read?
test/integration-test/src/tests/iter.rs line 23 at r8 (raw file):
assert_eq!(line_title, "tgid pid name"); // It's hard to predict what's the PID of the first process in a container.
can we make the assertion agnostic to the pid number but still check it? e.g. we could split each line on whitespace and assert we find init or systemd?
The recommendation (coming from rust-lang/rust#130062) for Linux hosts
is using C compiler driver as a linker, which is able to find
system-wide libraries. Using linker binaries directly in
-C linker(e.g.
-C linker=rust-lld) often results in errors like:Not touching the linker settings is usually the best approach for Linux
systems. Native builds pick up the default C toolchain. Cross builds
default to GCC cross wrapper, but that's easy to supress with clang
and lld using RUSTFLAGS.
However,
-C linker=rust-lldstill works the best on macOS, whereRust toolchains come with unwinder and runtime and there is usually no
need to link system libraries. Keep setting it only for macOS.
Fixes #907
This change is