Skip to content
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.

Sign up for GitHub

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

omicron-package: work around cargo issue 8157 #7218

Merged
merged 2 commits into from
Dec 10, 2024
+11 −1
Merged

omicron-package: work around cargo issue 8157 #7218

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b4cd5e
work around cargo issue 8157
iliana Dec 9, 2024
acbe39a
comments
iliana Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion package/src/bin/omicron-package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ struct Args {
#[derive(Debug, Default)]
struct CargoPlan<'a> {
command: &'a str,
packages: BTreeSet<&'a String>,
bins: BTreeSet<&'a String>,
features: BTreeSet<&'a String>,
release: bool,
Expand All @@ -123,6 +124,12 @@ impl<'a> CargoPlan<'a> {
// We rely on the rust-toolchain.toml file for toolchain information,
// rather than specifying one within the packaging tool.
cmd.arg(self.command);
// We specify _both_ --package and --bin; --bin does not imply
// --package, and without any --package options Cargo unifies features
// across all workspace default members. See rust-lang/cargo#8157.
for package in &self.packages {
cmd.arg("--package").arg(package);
}
for bin in &self.bins {
cmd.arg("--bin").arg(bin);
}
Expand Down Expand Up @@ -185,9 +192,12 @@ async fn do_for_all_rust_packages(
let mut debug = CargoPlan { command, release: false, ..Default::default() };

for (name, pkg) in config.packages_to_build().0 {
// If this is a Rust package...
// If this is a Rust package, `name` (the map key) is the name of the
// corresponding Rust crate.
if let PackageSource::Local { rust: Some(rust_pkg), .. } = &pkg.source {
let plan = if rust_pkg.release { &mut release } else { &mut debug };
// Add the package name to the plan
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment here about why this is necessary? Just a link to the issue or this PR would do.

Also worth noting somewhere (if not already done) that when source.rust is defined, the table key (e.g. the omicron-nexus in [package.omicron-nexus]) is the name of the corresponding Rust crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may also be useful to link to rust-lang/cargo#8157 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also worth noting somewhere (if not already done) that when source.rust is defined, the table key (e.g. the omicron-nexus in [package.omicron-nexus]) is the name of the corresponding Rust crate.

This is mentioned here:

omicron/package-manifest.toml

Lines 16 to 17 in d01b2ee

# If the package involves building a Rust package in the same workspace,
# then the Rust package *must* have the same name as the Omicron package.

but I agree it's non-obvious and it wouldn't hurt to mention it in other places where a reader might not know.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, in the comment here. I know the PR has it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

acbe39a does most of this; I think the particular detail about how to write package-manifest.toml is documented in the file itself:

omicron/package-manifest.toml

Lines 12 to 17 in d01b2ee

# (1) "local": packages whose contents come from any combination of files in the
# current directory, blobs stored in S3, or the result of building a Rust
# package in the current workspace
#
# If the package involves building a Rust package in the same workspace,
# then the Rust package *must* have the same name as the Omicron package.

But I think I improved the comments to make it clearer in that code's context.

plan.packages.insert(name);
// Get the package metadata
let metadata = workspace_pkgs.get(name).with_context(|| {
format!("package '{name}' is not a workspace package")
Expand Down
Loading