Skip to content

refactor(test/clitools)!: refine semantics of CliTestContext::kill_at()#4966

Draft
rami3l wants to merge 3 commits into
rust-lang:mainfrom
rami3l:refactor/polish-kill-at-checkpoint
Draft

refactor(test/clitools)!: refine semantics of CliTestContext::kill_at()#4966
rami3l wants to merge 3 commits into
rust-lang:mainfrom
rami3l:refactor/polish-kill-at-checkpoint

Conversation

@rami3l

@rami3l rami3l commented Jul 20, 2026

Copy link
Copy Markdown
Member

Refines #4963 before going further with e.g. #4965.

cc @cachebag

@cachebag cachebag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworked #4965 a bit to follow the patterns you have here. I will plan to rebase this onto it once it is merged. One thing maybe to note for myself it to drop my local copy of the metadata checkpoint name in favor ofCHECKPOINT_UPDATE_BEFORE_METADATA.

One question if you don't mind: for testing concurrent installs of the same toolchain, I'll need to keep a child parked at a checkpoint while a second command runs, then kill it explicitly. Would you be open to a spawn_at(checkpoint, args)-style primitive returning a guard (child killed on drop), with kill_at becoming a thin wrapper over it? I can propose that when I rebase.

View changes since this review

Comment thread src/test/clitools.rs
);
cmd.env(CHECKPOINT_ENV, checkpoint);

let _lock = CMD_LOCK.read().unwrap();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be dropped right after spawn()? Similar to as it already hapens here:

rustup/src/test/clitools.rs

Lines 479 to 481 in 09b779d

let lock = CMD_LOCK.read().unwrap();
let out = cmd.output();
drop(lock);

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