Skip to content

Commit 86b1195

Browse files
Add a gh-action and buildomat jobs to cargo check on no-default-features and feature-powerset (#6018)
Includes: * An xtask which - [X] runs with specific excludes, i.e. `image-*` - [X] downloads *pre-built* `cargo-hack` subcommand binary for known platform/os & arch from https://github.com/taiki-e/cargo-hack/releases/ - [X] allows for version-based installation (otherwise) * New CI jobs for checking-features in rust.yml & buildomat * Extends download `xtask` to install `cargo-hack`
1 parent 748a1d7 commit 86b1195

File tree

10 files changed

+379
-2
lines changed

10 files changed

+379
-2
lines changed

.cargo/config.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
# CI scripts:
99
# - .github/buildomat/build-and-test.sh
1010
# - .github/buildomat/jobs/clippy.sh
11+
# - .github/buildomat/jobs/check-features.sh
1112
# - .github/workflows/rust.yml
1213
#
1314
[build]
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
#!/bin/bash
2+
#:
3+
#: name = "check-features (helios)"
4+
#: variety = "basic"
5+
#: target = "helios-2.0"
6+
#: rust_toolchain = true
7+
#: output_rules = [
8+
#: "/out/*",
9+
#: ]
10+
11+
# Run the check-features `xtask` on illumos, testing compilation of feature combinations.
12+
13+
set -o errexit
14+
set -o pipefail
15+
set -o xtrace
16+
17+
cargo --version
18+
rustc --version
19+
20+
#
21+
# Set up our PATH for use with this workspace.
22+
#
23+
source ./env.sh
24+
export PATH="$PATH:$PWD/out/cargo-hack"
25+
26+
banner prerequisites
27+
ptime -m bash ./tools/install_builder_prerequisites.sh -y
28+
29+
#
30+
# Check feature combinations with the `cargo xtask check-features` command.
31+
#
32+
banner hack-check
33+
export CARGO_INCREMENTAL=0
34+
ptime -m timeout 2h cargo xtask check-features --ci

.github/buildomat/jobs/clippy.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
# (that we want to check) is conditionally-compiled on illumos only.
1111
#
1212
# Note that `cargo clippy` includes `cargo check, so this ends up checking all
13-
# of our code.
13+
# of our (default) code.
1414

1515
set -o errexit
1616
set -o pipefail

.github/workflows/rust.yml

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ jobs:
5353
run: cargo run --bin omicron-package -- -t default check
5454

5555
# Note that `cargo clippy` includes `cargo check, so this ends up checking all
56-
# of our code.
56+
# of our (default) code.
5757
clippy-lint:
5858
runs-on: ubuntu-22.04
5959
env:
@@ -82,6 +82,36 @@ jobs:
8282
- name: Run Clippy Lints
8383
run: cargo xtask clippy
8484

85+
check-features:
86+
runs-on: ubuntu-22.04
87+
env:
88+
CARGO_INCREMENTAL: 0
89+
steps:
90+
# This repo is unstable and unnecessary: https://github.com/microsoft/linux-package-repositories/issues/34
91+
- name: Disable packages.microsoft.com repo
92+
run: sudo rm -f /etc/apt/sources.list.d/microsoft-prod.list
93+
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
94+
with:
95+
ref: ${{ github.event.pull_request.head.sha }} # see omicron#4461
96+
- uses: Swatinem/rust-cache@23bce251a8cd2ffc3c1075eaa2367cf899916d84 # v2.7.3
97+
if: ${{ github.ref != 'refs/heads/main' }}
98+
- name: Report cargo version
99+
run: cargo --version
100+
- name: Update PATH
101+
run: |
102+
set -x
103+
export PATH="./out/cargo-hack:$PATH"
104+
source "./env.sh"; echo "PATH=$PATH" >> "$GITHUB_ENV"
105+
- name: Print PATH
106+
run: echo $PATH
107+
- name: Print GITHUB_ENV
108+
run: cat "$GITHUB_ENV"
109+
- name: Install Pre-Requisites
110+
run: ./tools/install_builder_prerequisites.sh -y
111+
- name: Run Check on Feature Combinations (Feature-Powerset, No-Dev-Deps)
112+
timeout-minutes: 120 # 2 hours
113+
run: cargo xtask check-features --ci
114+
85115
# This is just a test build of docs. Publicly available docs are built via
86116
# the separate "rustdocs" repo.
87117
build-docs:

README.adoc

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,21 @@ cargo nextest run
112112

113113
We check that certain system library dependencies are not leaked outside of their intended binaries via `cargo xtask verify-libraries` in CI. If you are adding a new dependency on a illumos/helios library it is recommended that you update xref:.cargo/xtask.toml[] with an allow list of where you expect the dependency to show up. For example some libraries such as `libnvme.so.1` are only available in the global zone and therefore will not be present in any other zone. This check is here to help us catch any leakage before we go to deploy on a rack. You can inspect a compiled binary in the target directory for what it requires by using `elfedit` - for example `elfedit -r -e 'dyn:tag NEEDED' /path/to/omicron/target/debug/sled-agent`.
114114

115+
=== Checking feature flag combinations
116+
117+
To ensure that varying combinations of features compile, run `cargo xtask check-features`, which executes the https://github.com/taiki-e/cargo-hack[`cargo hack`] subcommand under the hood.
118+
119+
This `xtask` is run in CI using the `--ci` parameter , which automatically exludes certain `image-*` features that purposefully cause compiler errors if set and uses a pre-built binary.
120+
121+
If `cargo hack` is not already installed in omicron's `out/` directory, a pre-built binary will be installed automatically depending on your operating system and architecture.
122+
123+
To limit the max number of simultaneous feature flags combined for checking, run the `xtask` with the `--depth <NUM>` flag:
124+
125+
[source,text]
126+
----
127+
$ cargo xtask check-features --depth 2
128+
----
129+
115130
=== Rust packages in Omicron
116131

117132
NOTE: The term "package" is overloaded: most programming languages and operating systems have their own definitions of a package. On top of that, Omicron bundles up components into our own kind of "package" that gets delivered via the install and update systems. These are described in the `package-manifest.toml` file in the root of the repo. In this section, we're just concerned with Rust packages.

dev-tools/xtask/src/check_features.rs

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
// This Source Code Form is subject to the terms of the Mozilla Public
2+
// License, v. 2.0. If a copy of the MPL was not distributed with this
3+
// file, You can obtain one at https://mozilla.org/MPL/2.0/.
4+
5+
//! Subcommand: cargo xtask check-features
6+
7+
use anyhow::{bail, Result};
8+
use camino::Utf8PathBuf;
9+
use clap::Parser;
10+
use std::{collections::HashSet, process::Command};
11+
12+
const SUPPORTED_ARCHITECTURES: [&str; 1] = ["x86_64"];
13+
const CI_EXCLUDED_FEATURES: [&str; 2] = ["image-trampoline", "image-standard"];
14+
15+
#[derive(Parser)]
16+
pub struct Args {
17+
/// Run in CI mode, with a default set of features excluded.
18+
#[clap(long, default_value_t = false)]
19+
ci: bool,
20+
/// Features to exclude from the check.
21+
#[clap(long, value_name = "FEATURES")]
22+
exclude_features: Option<Vec<String>>,
23+
/// Depth of the feature powerset to check.
24+
#[clap(long, value_name = "NUM")]
25+
depth: Option<usize>,
26+
/// Error format passed to `cargo hack check`.
27+
#[clap(long, value_name = "FMT")]
28+
message_format: Option<String>,
29+
/// Version of `cargo-hack` to install. By default, we download a pre-built
30+
/// version.
31+
#[clap(long, value_name = "VERSION")]
32+
install_version: Option<String>,
33+
}
34+
35+
/// Run `cargo hack check`.
36+
pub fn run_cmd(args: Args) -> Result<()> {
37+
// We cannot specify both `--ci` and `--install-version`, as the former
38+
// implies we are using a pre-built version.
39+
if args.ci && args.install_version.is_some() {
40+
bail!("cannot specify --ci and --install-version together");
41+
}
42+
43+
let cargo =
44+
std::env::var("CARGO").unwrap_or_else(|_| String::from("cargo"));
45+
46+
let mut command = Command::new(&cargo);
47+
48+
// Add the `hack check` subcommand.
49+
command.args(&["hack", "check"]);
50+
51+
if args.ci {
52+
install_prebuilt_cargo_hack(&cargo)?;
53+
54+
let ex = if let Some(mut features) = args.exclude_features {
55+
// Extend the list of features to exclude with the CI defaults.
56+
features.extend(
57+
CI_EXCLUDED_FEATURES.into_iter().map(|s| s.to_string()),
58+
);
59+
60+
// Remove duplicates.
61+
let excludes = features.into_iter().collect::<HashSet<_>>();
62+
63+
excludes.into_iter().collect::<Vec<_>>().join(",")
64+
} else {
65+
CI_EXCLUDED_FEATURES.join(",")
66+
};
67+
68+
// Add the `--exclude-features` flag if we are running in CI mode.
69+
command.args(["--exclude-features", &ex]);
70+
} else {
71+
install_cargo_hack(&cargo, args.install_version)?;
72+
// Add "only" the `--exclude-features` flag if it was provided.
73+
if let Some(features) = args.exclude_features {
74+
command.args(["--exclude-features", &features.join(",")]);
75+
}
76+
}
77+
78+
if let Some(depth) = args.depth {
79+
command.args(&["--depth", &depth.to_string()]);
80+
}
81+
82+
// Pass along the `--message-format` flag if it was provided.
83+
if let Some(fmt) = args.message_format {
84+
command.args(["--message-format", &fmt]);
85+
}
86+
87+
command
88+
// Make sure we check everything.
89+
.arg("--workspace")
90+
// We want to check the binaries.
91+
.arg("--bins")
92+
// We want to check the feature powerset.
93+
.arg("--feature-powerset")
94+
// We will not check the dev-dependencies, which should covered by tests.
95+
.arg("--no-dev-deps");
96+
97+
exec(command)
98+
}
99+
100+
/// The supported operating systems.
101+
enum Os {
102+
Illumos,
103+
Linux,
104+
Mac,
105+
}
106+
107+
/// Get the current OS.
108+
fn os_name() -> Result<Os> {
109+
let os = match std::env::consts::OS {
110+
"linux" => Os::Linux,
111+
"macos" => Os::Mac,
112+
"solaris" | "illumos" => Os::Illumos,
113+
other => bail!("OS not supported: {other}"),
114+
};
115+
Ok(os)
116+
}
117+
118+
/// This is a workaround for the lack of a CARGO_WORKSPACE_DIR environment
119+
/// variable, as suggested in <https://github.com/rust-lang/cargo/issues/3946#issuecomment-1433384192>.
120+
/// A better workaround might be to set this in the `[env]` section of
121+
/// `.cargo/config.toml`.
122+
fn project_root() -> Utf8PathBuf {
123+
Utf8PathBuf::from(&concat!(env!("CARGO_MANIFEST_DIR"), "/.."))
124+
}
125+
126+
/// Get the path to the `out` directory from the project root/workspace
127+
/// directory.
128+
fn out_dir() -> Utf8PathBuf {
129+
project_root().join("out/cargo-hack")
130+
}
131+
132+
/// Install `cargo-hack` if the `install-version` was specified; otherwise,
133+
/// download a pre-built version if it's not already in our `out` directory.
134+
fn install_cargo_hack(cargo: &str, version: Option<String>) -> Result<()> {
135+
if let Some(version) = version {
136+
let mut command = Command::new(cargo);
137+
138+
eprintln!(
139+
"installing cargo-hack at version {} to {}",
140+
version,
141+
env!("CARGO_HOME")
142+
);
143+
command.args(&["install", "cargo-hack", "--version", &version]);
144+
exec(command)
145+
} else if !out_dir().exists() {
146+
install_prebuilt_cargo_hack(cargo)
147+
} else {
148+
let out_dir = out_dir();
149+
eprintln!("cargo-hack found in {}", out_dir);
150+
Ok(())
151+
}
152+
}
153+
154+
/// Download a pre-built version of `cargo-hack` to the `out` directory via the
155+
/// download `xtask`.
156+
fn install_prebuilt_cargo_hack(cargo: &str) -> Result<()> {
157+
let mut command = Command::new(cargo);
158+
159+
let out_dir = out_dir();
160+
eprintln!(
161+
"cargo-hack not found in {}, downloading a pre-built version",
162+
out_dir
163+
);
164+
165+
let os = os_name()?;
166+
match os {
167+
Os::Illumos | Os::Linux | Os::Mac
168+
if SUPPORTED_ARCHITECTURES.contains(&std::env::consts::ARCH) =>
169+
{
170+
// Download the pre-built version of `cargo-hack` via our
171+
// download `xtask`.
172+
command.args(&["xtask", "download", "cargo-hack"]);
173+
}
174+
_ => {
175+
bail!(
176+
"cargo-hack is not pre-built for this os {} / arch {}",
177+
std::env::consts::OS,
178+
std::env::consts::ARCH
179+
);
180+
}
181+
}
182+
183+
exec(command)
184+
}
185+
186+
/// Execute the command and check the exit status.
187+
fn exec(mut command: Command) -> Result<()> {
188+
let cargo =
189+
std::env::var("CARGO").unwrap_or_else(|_| String::from("cargo"));
190+
191+
eprintln!(
192+
"running: {:?} {}",
193+
&cargo,
194+
command
195+
.get_args()
196+
.map(|arg| format!("{:?}", arg.to_str().unwrap()))
197+
.collect::<Vec<_>>()
198+
.join(" ")
199+
);
200+
201+
let exit_status = command
202+
.spawn()
203+
.expect("failed to spawn child process")
204+
.wait()
205+
.expect("failed to wait for child process");
206+
207+
if !exit_status.success() {
208+
bail!("cargo-hack install failed: {}", exit_status);
209+
}
210+
211+
Ok(())
212+
}

0 commit comments

Comments
 (0)