From 025c76780c0755fa02f9b4b6a9506453b9f56f6b Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Tue, 18 Jul 2023 15:26:05 -0400 Subject: [PATCH 1/4] integration-test: add to default-members This works now that build.rs does the right thing. Update the `miri test` command in the lint job so it has the proper exclusions; it is now in line with the invocations in the build-test job. --- .github/workflows/lint.yml | 9 ++++++++- Cargo.toml | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 16edbf5b..d26dd5a1 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -38,4 +38,11 @@ jobs: run: cargo hack clippy --all-targets --feature-powerset --workspace -- --deny warnings - name: Run miri - run: cargo miri test --all-targets + run: | + cargo hack miri test --all-targets --feature-powerset \ + --exclude aya-bpf \ + --exclude aya-bpf-bindings \ + --exclude aya-log-ebpf \ + --exclude integration-ebpf \ + --exclude integration-test \ + --workspace diff --git a/Cargo.toml b/Cargo.toml index 580e0a48..9bd39326 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,7 +29,7 @@ default-members = [ "aya-log-parser", "aya-obj", "aya-tool", - # test/integration-test is omitted; it must be built with xtask. + "test/integration-test", "xtask", "aya-bpf-macros", From 0168396604c15cb0fb7e065b0eeb6a57c05efefd Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Tue, 18 Jul 2023 16:25:13 -0400 Subject: [PATCH 2/4] integration-ebpf: add cargo config Same reasoning as the one in bpf. --- bpf/.cargo/config.toml | 2 +- test/integration-ebpf/.cargo/config.toml | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 test/integration-ebpf/.cargo/config.toml diff --git a/bpf/.cargo/config.toml b/bpf/.cargo/config.toml index 61fc56a9..d8d7a20c 100644 --- a/bpf/.cargo/config.toml +++ b/bpf/.cargo/config.toml @@ -6,7 +6,7 @@ # ignored if you run from the workspace root. See # https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure [build] -target = "bpfel-unknown-none" +target = ["bpfeb-unknown-none", "bpfel-unknown-none"] [unstable] build-std = ["core"] diff --git a/test/integration-ebpf/.cargo/config.toml b/test/integration-ebpf/.cargo/config.toml new file mode 100644 index 00000000..d8d7a20c --- /dev/null +++ b/test/integration-ebpf/.cargo/config.toml @@ -0,0 +1,12 @@ +# We have this so that one doesn't need to manually pass +# --target=bpfel-unknown-none -Z build-std=core when running cargo +# check/build/doc etc. +# +# NB: this file gets loaded only if you run cargo from this directory, it's +# ignored if you run from the workspace root. See +# https://doc.rust-lang.org/cargo/reference/config.html#hierarchical-structure +[build] +target = ["bpfeb-unknown-none", "bpfel-unknown-none"] + +[unstable] +build-std = ["core"] From e276c07f73f8eec94fb023c0652e6f0c084a0833 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Tue, 18 Jul 2023 16:17:46 -0400 Subject: [PATCH 3/4] integration-ebpf: invalidate on bpf-linker Extract the symlink-to-bpf-linker logic from integration-test to xtask and use it in a new build script in integration-ebpf, causing ebpf probes to be rebuilt when bpf-linker changes. Previously bpf-linker changes would rebuild integration-test, but not integration-ebpf, resulting in stale tests. Note that this still doesn't address the possibility that a new bpf-linker is added to the PATH ahead of the cached one. Solving this in the general case would require rebuild-if-changed-env=PATH *and* rebuild-if-changed={every-directory-in-PATH} which would likely mean far too much cache invalidation. --- test/integration-ebpf/Cargo.toml | 3 +++ test/integration-ebpf/build.rs | 23 +++++++++++++++++ test/integration-test/Cargo.toml | 1 - test/integration-test/build.rs | 44 ++++++++------------------------ xtask/Cargo.toml | 1 + xtask/src/lib.rs | 33 ++++++++++++++++++++++-- xtask/src/run.rs | 3 ++- 7 files changed, 71 insertions(+), 37 deletions(-) create mode 100644 test/integration-ebpf/build.rs diff --git a/test/integration-ebpf/Cargo.toml b/test/integration-ebpf/Cargo.toml index ff35ddb7..bb179c59 100644 --- a/test/integration-ebpf/Cargo.toml +++ b/test/integration-ebpf/Cargo.toml @@ -8,6 +8,9 @@ publish = false aya-bpf = { path = "../../bpf/aya-bpf" } aya-log-ebpf = { path = "../../bpf/aya-log-ebpf" } +[build-dependencies] +xtask = { path = "../../xtask" } + [[bin]] name = "log" path = "src/log.rs" diff --git a/test/integration-ebpf/build.rs b/test/integration-ebpf/build.rs new file mode 100644 index 00000000..35cb3582 --- /dev/null +++ b/test/integration-ebpf/build.rs @@ -0,0 +1,23 @@ +use std::{env, path::PathBuf}; + +use xtask::{create_symlink_to_binary, AYA_BUILD_INTEGRATION_BPF}; + +fn main() { + println!("cargo:rerun-if-env-changed={}", AYA_BUILD_INTEGRATION_BPF); + + let build_integration_bpf = env::var(AYA_BUILD_INTEGRATION_BPF) + .as_deref() + .map(str::parse) + .map(Result::unwrap) + .unwrap_or_default(); + + if build_integration_bpf { + let out_dir = env::var_os("OUT_DIR").unwrap(); + let out_dir = PathBuf::from(out_dir); + let bpf_linker_symlink = create_symlink_to_binary(&out_dir, "bpf-linker").unwrap(); + println!( + "cargo:rerun-if-changed={}", + bpf_linker_symlink.to_str().unwrap() + ); + } +} diff --git a/test/integration-test/Cargo.toml b/test/integration-test/Cargo.toml index bf82ab18..2a4aece6 100644 --- a/test/integration-test/Cargo.toml +++ b/test/integration-test/Cargo.toml @@ -25,5 +25,4 @@ tokio = { version = "1.24", default-features = false, features = [ [build-dependencies] cargo_metadata = { version = "0.15.4", default-features = false } -which = { version = "4.4.0", default-features = false } xtask = { path = "../../xtask" } diff --git a/test/integration-test/build.rs b/test/integration-test/build.rs index f41c4354..e0ceee48 100644 --- a/test/integration-test/build.rs +++ b/test/integration-test/build.rs @@ -12,21 +12,16 @@ use std::{ use cargo_metadata::{ Artifact, CompilerMessage, Dependency, Message, Metadata, MetadataCommand, Package, Target, }; -use which::which; -use xtask::{exec, LIBBPF_DIR}; +use xtask::{create_symlink_to_binary, exec, AYA_BUILD_INTEGRATION_BPF, LIBBPF_DIR}; fn main() { - const AYA_BUILD_INTEGRATION_BPF: &str = "AYA_BUILD_INTEGRATION_BPF"; - println!("cargo:rerun-if-env-changed={}", AYA_BUILD_INTEGRATION_BPF); - let build_integration_bpf = match env::var_os(AYA_BUILD_INTEGRATION_BPF) { - None => false, - Some(s) => { - let s = s.to_str().unwrap(); - s.parse::().unwrap() - } - }; + let build_integration_bpf = env::var(AYA_BUILD_INTEGRATION_BPF) + .as_deref() + .map(str::parse) + .map(Result::unwrap) + .unwrap_or_default(); const INTEGRATION_EBPF_PACKAGE: &str = "integration-ebpf"; @@ -137,28 +132,11 @@ fn main() { } } - // Create a symlink in the out directory to work around the fact that cargo ignores anything - // in `$CARGO_HOME`, which is also where `cargo install` likes to place binaries. Cargo will - // stat through the symlink and discover that bpf-linker has changed. - // - // This was introduced in https://github.com/rust-lang/cargo/commit/99f841c. - { - let bpf_linker = which("bpf-linker").unwrap(); - let bpf_linker_symlink = out_dir.join("bpf-linker"); - match fs::remove_file(&bpf_linker_symlink) { - Ok(()) => {} - Err(err) => { - if err.kind() != std::io::ErrorKind::NotFound { - panic!("failed to remove symlink: {err}") - } - } - } - std::os::unix::fs::symlink(&bpf_linker, &bpf_linker_symlink).unwrap(); - println!( - "cargo:rerun-if-changed={}", - bpf_linker_symlink.to_str().unwrap() - ); - } + let bpf_linker_symlink = create_symlink_to_binary(&out_dir, "bpf-linker").unwrap(); + println!( + "cargo:rerun-if-changed={}", + bpf_linker_symlink.to_str().unwrap() + ); let mut cmd = Command::new("cargo"); cmd.args([ diff --git a/xtask/Cargo.toml b/xtask/Cargo.toml index 6610e982..4614a92f 100644 --- a/xtask/Cargo.toml +++ b/xtask/Cargo.toml @@ -14,3 +14,4 @@ proc-macro2 = "1" quote = "1" syn = "2" tempfile = "3" +which = { version = "4.4.0", default-features = false } diff --git a/xtask/src/lib.rs b/xtask/src/lib.rs index 4cf871f5..09db4cbc 100644 --- a/xtask/src/lib.rs +++ b/xtask/src/lib.rs @@ -1,5 +1,13 @@ use anyhow::{anyhow, Context as _, Result}; -use std::process::Command; +use std::{ + fs, + path::{Path, PathBuf}, + process::Command, +}; +use which::which; + +pub const AYA_BUILD_INTEGRATION_BPF: &str = "AYA_BUILD_INTEGRATION_BPF"; +pub const LIBBPF_DIR: &str = "xtask/libbpf"; pub fn exec(cmd: &mut Command) -> Result<()> { let status = cmd @@ -14,4 +22,25 @@ pub fn exec(cmd: &mut Command) -> Result<()> { } } -pub const LIBBPF_DIR: &str = "xtask/libbpf"; +// Create a symlink in the out directory to work around the fact that cargo ignores anything +// in `$CARGO_HOME`, which is also where `cargo install` likes to place binaries. Cargo will +// stat through the symlink and discover that the binary has changed. +// +// This was introduced in https://github.com/rust-lang/cargo/commit/99f841c. +// +// TODO(https://github.com/rust-lang/cargo/pull/12369): Remove this when the fix is available. +pub fn create_symlink_to_binary(out_dir: &Path, binary_name: &str) -> Result { + let binary = which(binary_name).unwrap(); + let symlink = out_dir.join(binary_name); + match fs::remove_file(&symlink) { + Ok(()) => {} + Err(err) => { + if err.kind() != std::io::ErrorKind::NotFound { + return Err(err).context(format!("failed to remove symlink {}", symlink.display())); + } + } + } + std::os::unix::fs::symlink(binary, &symlink) + .with_context(|| format!("failed to create symlink {}", symlink.display()))?; + Ok(symlink) +} diff --git a/xtask/src/run.rs b/xtask/src/run.rs index 396d9ece..68f614a0 100644 --- a/xtask/src/run.rs +++ b/xtask/src/run.rs @@ -8,6 +8,7 @@ use std::{ use anyhow::{Context as _, Result}; use cargo_metadata::{Artifact, CompilerMessage, Message, Target}; use clap::Parser; +use xtask::AYA_BUILD_INTEGRATION_BPF; #[derive(Debug, Parser)] pub struct BuildOptions { @@ -35,7 +36,7 @@ pub struct Options { pub fn build(opts: BuildOptions) -> Result> { let BuildOptions { release, target } = opts; let mut cmd = Command::new("cargo"); - cmd.env("AYA_BUILD_INTEGRATION_BPF", "true").args([ + cmd.env(AYA_BUILD_INTEGRATION_BPF, "true").args([ "build", "--tests", "--message-format=json", From 6fc09ca07a663a73ed308b1bd794318d99342fe6 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Tue, 18 Jul 2023 14:57:40 -0400 Subject: [PATCH 4/4] integration-test: build-dep on integration-ebpf Remove the manual dependency tracking machinery in integration-test/build.rs in favor of a build-dependency on integration-ebpf. This required adding an empty lib.rs to create the library target. This allows integration-test/build.rs to be ignorant of bpf-linker. Remove that in favor of the logic now in integration-ebpf. --- test/integration-ebpf/src/lib.rs | 3 +++ test/integration-test/Cargo.toml | 13 ++++++++++ test/integration-test/build.rs | 43 ++++++-------------------------- 3 files changed, 24 insertions(+), 35 deletions(-) create mode 100644 test/integration-ebpf/src/lib.rs diff --git a/test/integration-ebpf/src/lib.rs b/test/integration-ebpf/src/lib.rs new file mode 100644 index 00000000..3ac3e595 --- /dev/null +++ b/test/integration-ebpf/src/lib.rs @@ -0,0 +1,3 @@ +#![no_std] + +// This file exists to enable the library target. diff --git a/test/integration-test/Cargo.toml b/test/integration-test/Cargo.toml index 2a4aece6..dc4bb88a 100644 --- a/test/integration-test/Cargo.toml +++ b/test/integration-test/Cargo.toml @@ -25,4 +25,17 @@ tokio = { version = "1.24", default-features = false, features = [ [build-dependencies] cargo_metadata = { version = "0.15.4", default-features = false } +# TODO(https://github.com/rust-lang/cargo/issues/12375): this should be an artifact dependency, but +# it's not possible to tell cargo to use `-Z build-std` to build it. We cargo-in-cargo in the build +# script to build this, but we want to teach cargo about the dependecy so that cache invalidation +# works properly. +# +# Note also that https://github.com/rust-lang/cargo/issues/10593 occurs when `target = ...` is added +# to an artifact dependency; it seems possible to work around that by setting `resolver = "1"` in +# Cargo.toml in the workspace root. +# +# Finally note that *any* usage of `artifact = ...` in *any* Cargo.toml in the workspace breaks +# workflows with stable cargo; stable cargo outright refuses to load manifests that use unstable +# features. +integration-ebpf = { path = "../integration-ebpf" } xtask = { path = "../../xtask" } diff --git a/test/integration-test/build.rs b/test/integration-test/build.rs index e0ceee48..b1d9e7db 100644 --- a/test/integration-test/build.rs +++ b/test/integration-test/build.rs @@ -1,5 +1,4 @@ use std::{ - collections::{HashMap, HashSet}, env, ffi::OsString, fmt::Write as _, @@ -10,9 +9,9 @@ use std::{ }; use cargo_metadata::{ - Artifact, CompilerMessage, Dependency, Message, Metadata, MetadataCommand, Package, Target, + Artifact, CompilerMessage, Message, Metadata, MetadataCommand, Package, Target, }; -use xtask::{create_symlink_to_binary, exec, AYA_BUILD_INTEGRATION_BPF, LIBBPF_DIR}; +use xtask::{exec, AYA_BUILD_INTEGRATION_BPF, LIBBPF_DIR}; fn main() { println!("cargo:rerun-if-env-changed={}", AYA_BUILD_INTEGRATION_BPF); @@ -23,16 +22,11 @@ fn main() { .map(Result::unwrap) .unwrap_or_default(); - const INTEGRATION_EBPF_PACKAGE: &str = "integration-ebpf"; - let Metadata { packages, .. } = MetadataCommand::new().no_deps().exec().unwrap(); - let packages: HashMap = packages + let integration_ebpf_package = packages .into_iter() - .map(|package| { - let Package { name, .. } = &package; - (name.clone(), package) - }) - .collect(); + .find(|Package { name, .. }| name == "integration-ebpf") + .unwrap(); let manifest_dir = env::var_os("CARGO_MANIFEST_DIR").unwrap(); let manifest_dir = PathBuf::from(manifest_dir); @@ -116,27 +110,8 @@ fn main() { let target = format!("{target}-unknown-none"); - // Teach cargo about our dependencies. - let mut visited = HashSet::new(); - let mut frontier = vec![INTEGRATION_EBPF_PACKAGE]; - while let Some(package) = frontier.pop() { - if !visited.insert(package) { - continue; - } - let Package { dependencies, .. } = packages.get(package).unwrap(); - for Dependency { name, path, .. } in dependencies { - if let Some(path) = path { - println!("cargo:rerun-if-changed={}", path.as_str()); - frontier.push(name); - } - } - } - - let bpf_linker_symlink = create_symlink_to_binary(&out_dir, "bpf-linker").unwrap(); - println!( - "cargo:rerun-if-changed={}", - bpf_linker_symlink.to_str().unwrap() - ); + let Package { manifest_path, .. } = integration_ebpf_package; + let integration_ebpf_dir = manifest_path.parent().unwrap(); let mut cmd = Command::new("cargo"); cmd.args([ @@ -150,8 +125,6 @@ fn main() { ]); // Workaround to make sure that the rust-toolchain.toml is respected. - let Package { manifest_path, .. } = packages.get(INTEGRATION_EBPF_PACKAGE).unwrap(); - let integration_ebpf_dir = manifest_path.parent().unwrap(); cmd.env_remove("RUSTUP_TOOLCHAIN") .current_dir(integration_ebpf_dir); @@ -209,7 +182,7 @@ fn main() { fs::write(&dst, []).unwrap_or_else(|err| panic!("failed to create {dst:?}: {err}")); } - let Package { targets, .. } = packages.get(INTEGRATION_EBPF_PACKAGE).unwrap(); + let Package { targets, .. } = integration_ebpf_package; for Target { name, kind, .. } in targets { if *kind != ["bin"] { continue;