Skip to content

Conversation

@cgwalters
Copy link
Collaborator

This mirrors exactly what we're doing in bootc, motivated by prep for shipping this in Fedora (including EPEL).

This mirrors exactly what we're doing in bootc, motivated
by prep for shipping this in Fedora (including EPEL).

Signed-off-by: Colin Walters <[email protected]>
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new RPM spec file for bcvk and significantly refactors the xtask packaging logic to support generating source tarballs and SRPMs. The changes are well-structured and mirror practices from the bootc project. My review focuses on the new Rust code in xtask. I've found a critical issue in the SRPM generation logic that would prevent it from working correctly, along with a few medium-severity issues related to robustness and efficiency. The suggestions aim to fix the bug and improve the code quality.

Comment on lines +268 to +282
{
let specin = File::open(format!("contrib/packaging/{NAME}.spec"))
.map(BufReader::new)
.context("Opening spec")?;
let mut o = File::create(td.join(format!("{NAME}.spec"))).map(BufWriter::new)?;
for line in specin.lines() {
let line = line?;
if line.starts_with("Version:") {
writeln!(o, "# Replaced by cargo xtask package-srpm")?;
writeln!(o, "Version: {v}")?;
} else {
writeln!(o, "{line}")?;
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The spec file generation logic within impl_srpm is incomplete. It only replaces the Version tag, but not Source0 and Source1. The template spec file contains URLs for these sources. Without replacing them with the local tarball filenames, rpmbuild will likely try to download them from the internet, which is not the intended behavior, or fail if it cannot resolve the macros. This logic should be updated to also replace Source0 and Source1 with the filenames from the pkg variable, similar to how update_spec does it.

    {
        let specin = File::open(format!("contrib/packaging/{NAME}.spec"))
            .map(BufReader::new)
            .context("Opening spec")?;
        let mut o = File::create(td.join(format!("{NAME}.spec"))).map(BufWriter::new)?;
        let srcpath_fname = pkg.srcpath.file_name().unwrap();
        let vendorpath_fname = pkg.vendorpath.file_name().unwrap();
        for line in specin.lines() {
            let line = line?;
            if line.starts_with("Version:") {
                writeln!(o, "# Replaced by cargo xtask package-srpm")?;
                writeln!(o, "Version: {v}")?;
            } else if line.starts_with("Source0") {
                writeln!(o, "Source0:        {}", srcpath_fname)?;
            } else if line.starts_with("Source1") {
                writeln!(o, "Source1:        {}", vendorpath_fname)?;
            } else {
                writeln!(o, "{line}")?;
            }
        }
    }

Comment on lines +136 to +140
let config = config.as_table_mut().unwrap();
let source_table = config.get_mut("source").unwrap();
let source_table = source_table.as_table_mut().unwrap();
let vendored_sources = source_table.get_mut("vendored-sources").unwrap();
let vendored_sources = vendored_sources.as_table_mut().unwrap();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This chain of unwrap() calls is brittle and will cause a panic if the structure of the cargo-vendor-filterer output TOML changes. While this is an internal build script, it's better to handle this gracefully with proper error handling. Consider using and_then or ? with ok_or_else to provide more informative error messages.

Suggested change
let config = config.as_table_mut().unwrap();
let source_table = config.get_mut("source").unwrap();
let source_table = source_table.as_table_mut().unwrap();
let vendored_sources = source_table.get_mut("vendored-sources").unwrap();
let vendored_sources = vendored_sources.as_table_mut().unwrap();
let vendored_sources = config
.as_table_mut()
.and_then(|c| c.get_mut("source"))
.and_then(|s| s.as_table_mut())
.and_then(|s| s.get_mut("vendored-sources"))
.and_then(|vs| vs.as_table_mut())
.ok_or_else(|| eyre!("malformed vendor config: missing [source.vendored-sources] table"))?;

// Add vendor config to source archive
cmd!(sh, "tar --owner=0 --group=0 --numeric-owner --sort=name --mtime=@{source_date_epoch} -rf {temp_tar} --transform='s|.*/.cargo-vendor-config.toml|bcvk-{version}/.cargo/vendor-config.toml|' {vendor_config_path}").run()?;
let p = Utf8Path::new("target");
let pkg = impl_package(sh)?;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The impl_package() function, which can be time-consuming as it creates tarballs, is called inside both update_spec() (here) and impl_srpm() (line 259). This means that running separate xtasks like spec and package-srpm will cause the packaging process to run multiple times unnecessarily. Consider refactoring to call impl_package() once per high-level task and pass the resulting Package struct to the helper functions. This would improve efficiency and make the data flow clearer.

let pkg = impl_package(sh)?;
let td = tempfile::tempdir_in("target").context("Allocating tmpdir")?;
let td = td.keep();
let td: &Utf8Path = td.as_path().try_into().unwrap();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of unwrap() here can cause a panic if the temporary directory path created by tempfile is not valid UTF-8. While unlikely on most systems, it's safer to handle this potential error explicitly by using ? and providing an error context.

Suggested change
let td: &Utf8Path = td.as_path().try_into().unwrap();
let td: &Utf8Path = td.as_path().try_into().context("temporary directory path is not valid UTF-8")?;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant