-
Notifications
You must be signed in to change notification settings - Fork 8
contrib/packaging: New spec file #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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]>
There was a problem hiding this 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.
| { | ||
| 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}")?; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}")?;
}
}
}| 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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")?; |
This mirrors exactly what we're doing in bootc, motivated by prep for shipping this in Fedora (including EPEL).