From 5b15d686c2ec902df7128102e429a9d1d3b3f1d6 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 1 Mar 2025 12:51:56 -0500 Subject: [PATCH 1/2] build: Tweak `make validate-rust` This took me some thinking and experimenting. Basically we want: - Hard deny some warnings (this is covered by the Cargo.toml workspace.lints.rust) - Gate merging to main in CI on an exact set of warnings we want to forbid, but *without* also using a blanket -Dwarning deny policy because that could break our build when the compiler revs. - A corollary to the previous: allow developing locally without killing the build just because you have an unused import or some dead code (for example). So we don't want to add `dead_code = deny` into the Cargo.toml. - Be able to easily reproduce locally what CI is gating on in an efficient way. We already had `make validate-rust` which was intending to navigate this, but what was missing was the "deny extended set of warnings" so we got code committed to git main which hit `unused_imports`. Clippy upstream docs recommend the `RUSTFLAGS = -Dwarnings` approach in e.g. https://doc.rust-lang.org/clippy/continuous_integration/github_actions.html but again I think this is a problem because it can break with updated Rust/clippy versions (unless you pin on those, but that becomes a pain in and of itself). The problem also with doing `RUSTFLAGS = -Dwarnings` *locally* is it blows out the cargo cache. So here's the solution I came to: We run `cargo clippy -A clippy:all`, and then deny some specific clippy lints *and* the core Rust warnings we want (`unused_imports` type things) at this stage. The advantage is this doesn't blow out the main Cargo cache, and I can easily reproduce locally exactly what CI would gate on. Also while we're here, add `make fix-rust` which is a handy way to use the existing `clippy --fix` to locally fix things like unused imports as well as other machine-applicable things that are in e.g. `clippy::suspicious`. Signed-off-by: Colin Walters --- Makefile | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index feb3679b..8d515eb8 100644 --- a/Makefile +++ b/Makefile @@ -59,15 +59,22 @@ test-bin-archive: all test-tmt: cargo xtask test-tmt -# Checks extra rust things (formatting, a few extra rust warnings, and select clippy lints) +# This gates CI by default. Note that for clippy, we gate on +# only the clippy correctness and suspicious lints, plus a select +# set of default rustc warnings. +# We intentionally don't gate on this for local builds in cargo.toml +# because it impedes iteration speed. +CLIPPY_CONFIG = -A clippy::all -D clippy::correctness -D clippy::suspicious -Dunused_imports -Ddead_code validate-rust: cargo fmt -- --check -l - cargo check - (cd lib && cargo check --no-default-features) cargo test --no-run - cargo clippy -- -D clippy::correctness -D clippy::suspicious + (cd lib && cargo check --no-default-features) + cargo clippy -- $(CLIPPY_CONFIG) env RUSTDOCFLAGS='-D warnings' cargo doc --lib .PHONY: validate-rust +fix-rust: + cargo clippy --fix --allow-dirty -- $(CLIPPY_CONFIG) +.PHONY: fix-rust validate: validate-rust ruff check From 112e36d7591c829312692fc03ffd1e7beb54171c Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 28 Feb 2025 13:26:30 -0500 Subject: [PATCH 2/2] install: Add a generic `install finalize` Basically I want to get Anaconda to run this, then we can perform arbitrary fixups on whatever it did between the install and reboot without changing Anaconda's code. This also applies to user `%post` scripts for example; maybe those break the bootloader entries in /boot; we have the opportunity to catch such things here. Or we may choose to start forcibly relabeling the target `/etc`. Signed-off-by: Colin Walters --- docs/src/bootc-install.md | 20 ++++++++++++++++++++ lib/src/cli.rs | 9 +++++++++ lib/src/install.rs | 21 +++++++++++++++++++++ tests-integration/src/install.rs | 7 +++++++ 4 files changed, 57 insertions(+) diff --git a/docs/src/bootc-install.md b/docs/src/bootc-install.md index 512aee23..1614be3e 100644 --- a/docs/src/bootc-install.md +++ b/docs/src/bootc-install.md @@ -161,6 +161,26 @@ storage or filesystem setups, but reuses the "top half" of the logic. For example, a goal is to change [Anaconda](https://github.com/rhinstaller/anaconda/) to use this. +#### Postprocessing after to-filesystem + +Some installation tools may want to inject additional data, such as adding +an `/etc/hostname` into the target root. At the current time, bootc does +not offer a direct API to do this. However, the backend for bootc is +ostree, and it is possible to enumerate the deployments via ostree APIs. + +We hope to provide a bootc-supported method to find the deployment in +the future. + +However, for tools that do perform any changes, there is a new +`bootc install finalize` command which is optional, but recommended +to run as the penultimate step before unmounting the target filesystem. + +This command will perform some basic sanity checks and may also +perform fixups on the target root. For example, a direction +currently for bootc is to stop using `/etc/fstab`. While `install finalize` +does not do this today, in the future it may automatically migrate +`etc/fstab` to `rootflags` kernel arguments. + ### Using `bootc install to-disk --via-loopback` Because every `bootc` system comes with an opinionated default installation diff --git a/lib/src/cli.rs b/lib/src/cli.rs index 958c9cbe..bfac767b 100644 --- a/lib/src/cli.rs +++ b/lib/src/cli.rs @@ -209,6 +209,12 @@ pub(crate) enum InstallOpts { /// will be wiped, but the content of the existing root will otherwise be retained, and will /// need to be cleaned up if desired when rebooted into the new root. ToExistingRoot(crate::install::InstallToExistingRootOpts), + /// Execute this as the penultimate step of an installation using `install to-filesystem`. + /// + Finalize { + /// Path to the mounted root filesystem. + root_path: Utf8PathBuf, + }, /// Intended for use in environments that are performing an ostree-based installation, not bootc. /// /// In this scenario the installation may be missing bootc specific features such as @@ -1121,6 +1127,9 @@ async fn run_from_opt(opt: Opt) -> Result<()> { let rootfs = &Dir::open_ambient_dir("/", cap_std::ambient_authority())?; crate::install::completion::run_from_anaconda(rootfs).await } + InstallOpts::Finalize { root_path } => { + crate::install::install_finalize(&root_path).await + } }, Opt::ExecInHostMountNamespace { args } => { crate::install::exec_in_host_mountns(args.as_slice()) diff --git a/lib/src/install.rs b/lib/src/install.rs index 722e3642..2495f094 100644 --- a/lib/src/install.rs +++ b/lib/src/install.rs @@ -1452,6 +1452,9 @@ async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Re // descriptors. } + // Run this on every install as the penultimate step + install_finalize(&rootfs.physical_root_path).await?; + // Finalize mounted filesystems if !rootfs.skip_finalize { let bootfs = rootfs.boot.as_ref().map(|_| ("boot", "boot")); @@ -1909,6 +1912,24 @@ pub(crate) async fn install_to_existing_root(opts: InstallToExistingRootOpts) -> install_to_filesystem(opts, true).await } + +/// Implementation of `bootc install finalize`. +pub(crate) async fn install_finalize(target: &Utf8Path) -> Result<()> { + crate::cli::require_root(false)?; + let sysroot = ostree::Sysroot::new(Some(&gio::File::for_path(target))); + sysroot.load(gio::Cancellable::NONE)?; + let deployments = sysroot.deployments(); + // Verify we find a deployment + if deployments.is_empty() { + anyhow::bail!("Failed to find deployment in {target}"); + } + + // For now that's it! We expect to add more validation/postprocessing + // later, such as munging `etc/fstab` if needed. See + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/tests-integration/src/install.rs b/tests-integration/src/install.rs index cdba047b..06c30e2b 100644 --- a/tests-integration/src/install.rs +++ b/tests-integration/src/install.rs @@ -104,6 +104,13 @@ pub(crate) fn run_alongside(image: &str, mut testargs: libtest_mimic::Arguments) std::fs::write(&tmp_keys, b"ssh-ed25519 ABC0123 testcase@example.com")?; cmd!(sh, "sudo {BASE_ARGS...} {target_args...} -v {tmp_keys}:/test_authorized_keys {image} bootc install to-filesystem {generic_inst_args...} --acknowledge-destructive --karg=foo=bar --replace=alongside --root-ssh-authorized-keys=/test_authorized_keys /target").run()?; + // Also test install finalize here + cmd!( + sh, + "sudo {BASE_ARGS...} {target_args...} {image} bootc install finalize /target" + ) + .run()?; + generic_post_install_verification()?; // Test kargs injected via CLI