-
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: allow building specific packages and their dependencies #5799
Conversation
I ended up rewriting the dependency walker to avoid walking each node more than once. I also ran into an issue while testing this -- Cargo absolutely refuses to run if the requested features in While doing that refactor, I decided to change it to run |
"error: bin target {:?} seen multiple times", | ||
target.name |
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.
Dumb question, but when would this practically trigger? Mechanically, I understand that it's "a binary target exists in the set of all targets multiple times", but would this be like, multiple versions of one binary? How could that happen?
(Mostly asking for the sake of whoever sees this error in the wild - I'm also interested in whatever hint we could provide for "how to fix this").
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.
Two binaries with the same name in the workspace (e.g. some package has a binary named "schema-updater" along with the omicron-nexus package) — while self-reviewing the code I tested to see if Cargo dealt with that, and it... doesn't! (yet, at least.)
I can see how someone might do this accidentally, and if so it might get really hard to debug.
package/src/bin/omicron-package.rs
Outdated
if self.only.is_empty() { | ||
packages |
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.
nitty nitpick: This could probably be stated as:
if self.only.is_empty() {
return packages;
}
... // Contents of "else" block
To reduce some indentation
@@ -127,14 +127,7 @@ name = "sled-agent" | |||
doc = false | |||
|
|||
[features] | |||
image-standard = [] |
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 clarify, are all these features getting removed because they aren't used anywhere?
(If so, great, awesome to see these cleaned up)
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.
Yep, aiui these were all in sled-agent because the features had to be defined in some package for the cargo build
to work. But if you're not building sled-agent, you'll often get a failure to build because of features that don't exist in any of the selected packages.
I grepped for the feature names across the tree to identify which packages were actually using which feature names.
#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: - Without `--package`, Cargo uses the `default-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 to `default-members` and has a normal dependency on `nexus-test-utils`, which enables the `"testing"` feature of `nexus-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 the `cargo build --unit-graph` and `nm target/release/nexus | demangle | grep validate_volume_invar` no longer shows any matching testing-only symbols.
This change allows building a specific set of packages, such as the packages that are installed directly in the host OS image (as opposed to the zones that are installed separately).
The output doesn't make it clear, but a temporary
println!
debug statement demonstrates that this only built the necessary package:This is to unlock some speed improvements to
cargo xtask releng
so that the Nexus binary can be built at the same time as the host and recovery OS images. (In practice this is more useful for local rebuilds; at present the releng CI time is reduced by about 90 seconds, although reordering the Nexus build to occur simultaneously with OS image builds has revealed other wasted time.) The actual releng changes will be in another PR along with some other improvements.A more realistic invocation, with all the packages we need for the host OS image:
This is not the best experience at the moment; if any of the target-specific feature flags are not provided by any of the packages selected, Cargo complains:
We could try to use
cargo-metadata
to go through and see if any of the enabled packages have those features, or go add the requisite feature flags to the rest of the crates in the tree built by omicron-package. (Or add them to omicron-workspace-hack???)This is sort of an early revision of this PR; the actual implementation of selecting particular trees from a
PackageMap
maybe belongs in omicron-zone-package, but I don't have a good vibe on whether that's the case.