-
Notifications
You must be signed in to change notification settings - Fork 40
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
omicron-package: work around cargo issue 8157 #7218
Conversation
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.
thanks for jumping so quickly on this!
@@ -188,6 +192,8 @@ async fn do_for_all_rust_packages( | |||
// If this is a Rust package... | |||
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 |
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.
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.
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.
It may also be useful to link to rust-lang/cargo#8157 ?
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.
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:
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.
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.
To be clear, in the comment here. I know the PR has it!
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.
acbe39a does most of this; I think the particular detail about how to write package-manifest.toml is documented in the file itself:
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.
#5799 modified the
cargo build
command line omicron-package runs. Previously it built up a list of packages to be built using the-p
flag; that PR changed it to use--bin
. The goal was to build only the binaries that are necessary for shipping; this avoids building sled-agent-sim during releng, for instance.We did not realize it at the time, but this invited the specter of rust-lang/cargo#8157 to wreak havoc; namely:
--package
, Cargo uses thedefault-members
key of the workspace Cargo.toml to determine which packages to build.--bin
does not cause the same thing to happen; saying--bin
does not imply--package [the package that the bin belongs to]
.omicron-dev
belongs todefault-members
and has a normal dependency onnexus-test-utils
, which enables the"testing"
feature ofnexus-db-queries
.#7208 is a known result of this problem, but there might be more.
Fortunately the solution seems fairly easy, without reverting the relevant changes from #5799: use both
--package
and--bin
. With this change, the"testing"
feature is no longer shown in thecargo build --unit-graph
andnm target/release/nexus | demangle | grep validate_volume_invar
no longer shows any matching testing-only symbols.For posterity, the full command omicron-package runs is now (from the log):
"cargo" "build" "--package" "dns-server" "--package" "internal-dns-cli" "--package" "omicron-clickhouse-admin" "--package" "omicron-cockroach-admin" "--package" "omicron-gateway" "--package" "omicron-nexus" "--package" "omicron-omdb" "--package" "omicron-sled-agent" "--package" "oximeter-collector" "--package" "oxlog" "--package" "sp-sim" "--package" "wicket" "--package" "wicketd" "--package" "zone-setup" "--bin" "clickhouse-admin-keeper" "--bin" "clickhouse-admin-server" "--bin" "clickhouse-admin-single" "--bin" "clickhouse-schema-updater" "--bin" "cockroach-admin" "--bin" "dns-server" "--bin" "dnsadm" "--bin" "dnswait" "--bin" "mgs" "--bin" "nexus" "--bin" "omdb" "--bin" "oximeter" "--bin" "oxlog" "--bin" "schema-updater" "--bin" "sled-agent" "--bin" "sp-sim" "--bin" "wicket" "--bin" "wicketd" "--bin" "zone-bundle" "--bin" "zone-setup" "--features" "switch-asic" "--release"