-
Notifications
You must be signed in to change notification settings - Fork 22
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
PHD: allow patched Crucible dependencies #778
Conversation
`phd-runner` uses the dependency on Crucible specified in the workspace's Cargo.toml to determine which downstairs artifact to download from Buildomat. This only works when the Crucible dependency is a Git dep on the `oxidecomputer/crucible` GitHub repo. In some cases, developers may patch Crucible to a local checkout or a different repository, such as for testing changes that haven't been merged to `oxidecomputer/crucible`. Unfortunately, the phd-runner build script fails in this case, which makes running any PHD tests impossible. This is unfortunate. PHD tests _can_ still be run when the Crucible dependency is patched, either by disabling the Crucible-based tests, or by manually providing a Crucible downstairs binary path. However, because the build script fails in this case, it's impossible to build `phd-runner`, which sucks. Instead, we should allow the build to succeed and just disable the `--crucible-downstairs-commit auto` CLI option when the dependency isn't pointed at the Crucible GitHub repo. This commit does that. Now, we just log a warning and set the env var to a sentinel value that means we couldn't determine the SHA for auto mode. The `phd-runner` binary will then exit with an error if `--crucible-downstairs-commit auto` is selected when compiled without a known Git SHA. To test this, I patched Crucible with a local checkout: ```toml [patch."https://github.com/oxidecomputer/crucible"] crucible = { path = "../crucible/upstairs" } crucible-client-types = { path = "../crucible/crucible-client-types" } ``` Running `cargo build -p phd-runner` now succeeds: ```console eliza@theseus ~/Code/oxide/propolis $ cargo build -p phd-runner Locking 4 packages to latest compatible versions Adding crucible v0.0.1 (/home/eliza/Code/oxide/crucible/upstairs) Adding crucible-client-types v0.1.0 (/home/eliza/Code/oxide/crucible/crucible-client-types) Adding crucible-common v0.0.1 (/home/eliza/Code/oxide/crucible/common) Adding crucible-protocol v0.0.0 (/home/eliza/Code/oxide/crucible/protocol) Compiling propolis-client v0.1.0 (/home/eliza/Code/oxide/propolis/lib/propolis-client) Compiling phd-runner v0.1.0 (/home/eliza/Code/oxide/propolis/phd-tests/runner) warning: [email protected]: Crucible upstairs dependency is not pointed at the https://github.com/oxidecomputer/crucible repository, so this phd-runner build not be able to automatically determine the Crucible commit to download from Buildomat: Crucible dependency is patched with a local checkout Compiling phd-framework v0.1.0 (/home/eliza/Code/oxide/propolis/phd-tests/framework) Compiling phd-testcase v0.1.0 (/home/eliza/Code/oxide/propolis/phd-tests/testcase) Compiling phd-tests v0.1.0 (/home/eliza/Code/oxide/propolis/phd-tests/tests) Finished `dev` profile [unoptimized + debuginfo] target(s) in 11.63s ``` Fixes #770
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.
Off-the-wall idea here, but: is it possible to get cargo xtask phd
to notice that the Crucible path has been patched and to build a downstairs binary from that path? We could then try to pipe it into a (separate) environment var and have --crucible-downstairs-commit auto
try to pick that up: the git rev isn't valid, but I have a proposed path to a Crucible downstairs on the local machine, so let me try that instead.
If that's intractable for some reason (and I could totally believe it is, because e.g. we don't really know if the Crucible dep is local or we don't want to build other projects like that while building this one), then this LGTM as-is.
phd-tests/runner/src/config.rs
Outdated
"Crucible upstairs dependency was patched with a \ | ||
local path when PHD was compiled, so the \ | ||
`--crucible-downstairs-commit auto` option has been \ | ||
disabled." |
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.
If we can't or don't want to go the "try to infer where the binary should be based on the path to the checkout" route, perhaps this message (and/or the one in build.rs) should advertise the --crucible-downstairs-cmd
option so that folks building a local Crucible know they can run PHD with 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.
Yeah, that's a good idea.
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 now says:
Error: Because Crucible dependency is patched with a local checkout, phd-runner's build script could not determine the Crucible Git SHA, so the `--crucible-downstairs-commit auto` option has been disabled. You can provide a local Crucible binary using `--crucible-downstairs-command`.
Hmm, that's a good idea, and I'm sure it's possible, although it might be a bit annoying. I have a slight preference for merging this change and then going and doing that, though, because the current state of the world is totally broken if you've patched Crucible, and I'd like |
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 new message looks great--thanks for tweaking it!
@gjcolombo I've opened #779 to track having |
phd-runner
uses the dependency on Crucible specified in the workspace's Cargo.toml to determine which downstairs artifact to download from Buildomat. This only works when the Crucible dependency is a Git dep on theoxidecomputer/crucible
GitHub repo. In some cases, developers may patch Crucible to a local checkout or a different repository, such as for testing changes that haven't been merged tooxidecomputer/crucible
. Unfortunately, the phd-runner build script fails in this case, which makes running any PHD tests impossible.This is unfortunate. PHD tests can still be run when the Crucible dependency is patched, either by disabling the Crucible-based tests, or by manually providing a Crucible downstairs binary path. However, because the build script fails in this case, it's impossible to build
phd-runner
, which sucks. Instead, we should allow the build to succeed and just disable the--crucible-downstairs-commit auto
CLI option when the dependency isn't pointed at the Crucible GitHub repo.This commit does that. Now, we just log a warning and set the env var to a sentinel value that means we couldn't determine the SHA for auto mode. The
phd-runner
binary will then exit with an error if--crucible-downstairs-commit auto
is selected when compiled without a known Git SHA.To test this, I patched Crucible with a local checkout:
Running
cargo build -p phd-runner
now succeeds:Fixes #770