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.

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

Merged
merged 10 commits into from
Jun 24, 2024

Conversation

iliana
Copy link
Contributor

@iliana iliana commented May 21, 2024

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).

$ cargo run --release --bin omicron-package -- package --only omicron-sled-agent
    Finished release [optimized] target(s) in 0.99s
     Running `target/release/omicron-package package --only omicron-sled-agent`
Logging to: /home/iliana/git/omicron/out/LOG
    Finished release [optimized] target(s) in 0.94s
[ 0s] ##############################       8/8       omicron-sled-agent: done

The output doesn't make it clear, but a temporary println! debug statement demonstrates that this only built the necessary package:

Command { std: "cargo" "build" "-p" "omicron-sled-agent" "--features" "image-standard machine-gimlet rack-topology-multi-sled switch-asic " "--release", kill_on_drop: false }

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:

$ cargo run --release --bin omicron-package -- package --only mg-ddm-gz --only omicron-sled-agent \
>    --only overlay --only oxlog --only propolis-server --only pumpkind-gz --only switch-asic
    Finished release [optimized] target(s) in 1.02s
     Running `target/release/omicron-package package --only mg-ddm-gz --only omicron-sled-agent --only overlay --only oxlog --only propolis-server --only pumpkind-gz --only switch-asic`
Logging to: /home/iliana/git/omicron/out/LOG
    Finished release [optimized] target(s) in 1.06s
[ 0s] ##############################       1/1       pumpkind: done
[ 0s] ##############################       3/3       oxlog: done
[ 1s] ##############################       1/1       mgd: done
[ 1s] ##############################       1/1       mg-ddm-gz: done
[ 0s] ##############################      10/10      omicron-gateway-asic-customizations: done
[ 0s] ##############################      12/12      wicket: done
[ 1s] ##############################       1/1       propolis-server: done
[ 0s] ##############################      64/64      switch_zone_setup: done
[ 1s] ##############################       1/1       mg-ddm: done
[ 0s] ##############################       8/8       omicron-gateway: done
[ 0s] ##############################       1/1       lldp: done
[ 0s] ##############################      11/11      logadm: done
[ 0s] ##############################       7/7       xcvradm: done
[ 0s] ##############################      16/16      wicketd: done
[ 1s] ##############################       1/1       pumpkind-gz: done
[ 0s] ##############################       8/8       omicron-omdb: done
[ 0s] ##############################       8/8       profile: done
[ 0s] ##############################       8/8       omicron-sled-agent: done
[ 1s] ##############################       1/1       dendrite-asic: done
[ 0s] ##############################       4/4       overlay: done
[ 0s] ##############################       4/4       omicron-gateway-asic: done
[ 0s] ##############################      13/13      switch-asic: done

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:

$ cargo run --release --bin omicron-package -- package --only switch-asic
    Finished release [optimized] target(s) in 0.98s
     Running `target/release/omicron-package package --only switch-asic`
Logging to: /home/iliana/git/omicron/out/LOG
error: none of the selected packages contains these features: image-standard, machine-gimlet, rack-topology-multi-sled, switch-asic
Error: Failed to build packages

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.

@iliana iliana requested a review from smklein May 21, 2024 02:32
package/src/bin/omicron-package.rs Outdated Show resolved Hide resolved
package/src/bin/omicron-package.rs Outdated Show resolved Hide resolved
package/src/bin/omicron-package.rs Show resolved Hide resolved
package/src/bin/omicron-package.rs Outdated Show resolved Hide resolved
@iliana iliana requested a review from smklein June 21, 2024 20:50
@iliana
Copy link
Contributor Author

iliana commented Jun 21, 2024

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 --features aren't present in one of the requested packages. I tried a few workarounds but ultimately settled on filtering the feature list based on output from the cargo_metadata crate.

While doing that refactor, I decided to change it to run cargo build --bin [binaries..] instead of cargo build -p [packages..], explicitly requesting the targets that omicron-package actually wants.

Comment on lines +156 to +157
"error: bin target {:?} seen multiple times",
target.name
Copy link
Collaborator

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").

Copy link
Contributor Author

@iliana iliana Jun 24, 2024

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.

Comment on lines 980 to 981
if self.only.is_empty() {
packages
Copy link
Collaborator

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 = []
Copy link
Collaborator

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)

Copy link
Contributor Author

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.

@iliana iliana enabled auto-merge (squash) June 24, 2024 17:24
@iliana iliana merged commit 895f280 into main Jun 24, 2024
20 checks passed
@iliana iliana deleted the iliana/package-only branch June 24, 2024 18:21
iliana added a commit that referenced this pull request Dec 10, 2024
#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.
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.

2 participants