"external xtasks" pattern; wire up cargo xtask releng
#5783
Conversation
Could you just have the tool create child datasets for each of these under a single parent? |
|
I wanted to avoid writing anything in this tool that would pfexec directly. Also the default values for each is |
dev-tools/releng/src/main.rs
Outdated
| /// Run the Oxide release engineering process and produce a TUF repo that can be | ||
| /// used to update a rack. | ||
| /// | ||
| /// For more information, see `docs/releng.adoc` in the Omicron repository. | ||
| /// | ||
| /// Note that `--host-dataset` and `--recovery-dataset` must be set to different | ||
| /// values to build the two OS images in parallel. This is strongly recommended. | ||
| #[derive(Parser)] | ||
| #[command(name = "cargo xtask releng")] |
There was a problem hiding this comment.
Is this here to give better error messages when this command is invoked via xtask?
It's possible that the command is still cargo run explicitly, instead of being invoked by xtask, right?
There was a problem hiding this comment.
I thought this adjusted the usage string because this is the same thing that's in xtask, but it actually doesn't -- this appears to just be the name of the tool, which defaults to the crate name. So I'm going to try and find the correct thing and adjust that here and in xtask.
dev-tools/xtask/src/external.rs
Outdated
| #[clap(skip)] | ||
| command: Option<Command>, |
There was a problem hiding this comment.
If these args are being skipped, does it make sense to just omit this from the parsed struct?
dev-tools/xtask/src/external.rs
Outdated
| mut self, | ||
| args: impl IntoIterator<Item = impl AsRef<OsStr>>, | ||
| ) -> External { | ||
| self.command.get_or_insert_with(default_command).args(args); |
There was a problem hiding this comment.
Related to my comment above, this is calling .get_or_insert_with, but it's always using default_command, right?
| self.command.get_or_insert_with(default_command).args(args); | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
Rather than calling cargo_args and exec, we could just call:
pub struct External {
args: Vec<OsString>,
}
impl External {
pub fn exec(self, binary: impl AsRef<OsStr>) -> Result<()> {
default_command() // I might rename this to "cargo_run_command()"
.arg("--bin")
.arg(binary)
.arg("--")
.args(self.args)
.exec();
...
}
}I think this has the same functionality, but makes it a little clear that the "default" command is not actually modifiable.
There was a problem hiding this comment.
Yeah, this is what I was wondering too -- any particular reason to have command be part of the struct rather than creating it here?
There was a problem hiding this comment.
What this is missing is the ability to add --release to the command, which is particularly useful for the releng tooling. (I don't expect that all external xtasks should be built and run in release mode.)
There was a problem hiding this comment.
Specifically I imagine the usual flow in main.rs would be:
Cmds::Whatever(external) => external.exec("omicron-whatever"),What the Option<Command>, defualt_command, etc chicanery gets us is the ability to optionally add additional arguments to cargo itself, which we use for Cmds::Releng:
Cmds::Releng(external) => external.cargo_args(["--release"]).exec("omicron-releng"),
| self.command.get_or_insert_with(default_command).args(args); | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
Yeah, this is what I was wondering too -- any particular reason to have command be part of the struct rather than creating it here?
|
I'll add some comments to the |
I finally found the set of Clap options to do what I wanted!