Add tracked command abstraction to bootstrap #126564
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
jieyouxu
reviewed
Jun 16, 2024
| /// Runs the command. | ||
| #[track_caller] | ||
| pub fn run_maybe(&mut self) -> CommandOutput { | ||
| let output = self.cmd.output().expect("Cannot execute process"); |
There was a problem hiding this comment.
Suggestion: it might be nice to report the complete command invocation that failed
Noratrieb
reviewed
Jun 17, 2024
| use std::path::Path; | ||
|
|
||
| /// Command that checks whether git works at the given `directory`. | ||
| pub fn cmd_works(directory: &Path) -> TrackedCommand { |
|
Are we still working on this? #126731 makes this PR redundant if I am not mistaken. |
|
Yeah, we can close in favour of #126731. |
onur-ozkan
removed
the
S-waiting-on-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Boostrap currently accesses its external environment (the filesystem, process spawning etc.) in completely arbitrary ways, where essentially any function in bootstrap can just invoke a command. While this makes programming easier, it also makes bootstrap quite challenging to test, debug and profile.
As one possible improvement, it would be great if we could route all command invocations through a single location. This would enable us to log all command invocations, measure how long do they take, and in the future even potentially mock them somehow, to simplify testing.
For that, we need to have some custom representation of commands, using
std::process::Commanddiretly won't do. There is alreadyBootstrapCommand, but that is mostly just a very light refactoring of code that was spawning commands throughBuild(er), with various idiosyncracies (like the output and fail modes). In this PR, I added a newTrackedCommandstruct, with which I'd ultimately like to replace all currentCommandinvocations in bootstrap (step 1), and then also the current commands that are already routed through Build(er) (step 2, after whichBootstrapCommandcan be removed).The first commits adds the command struct (with a few basic APIs), while the second commit demonstrates its usage in the
channel.rsfile.Once everything is routed through
TrackedCommand, we can useclippyto disable creating new commands outside ofTrackedCommanditself, which should ensure that really everything will be routed through this struct.Apart from executing commands using this API, we should also ideally thread all command invocations through a single location (context). This would be useful for profiling (measuring how long each command execution took, and storing it into bootstrap build metrics), caching (hashing idempotent commands and storing their cached output, to avoid rerunning e.g. 20 identical git commands in one bootstrap invocation) and logging (this one could be done without a shared context, but it would again be essentially abusing global variables). While logging and profiling could be done e.g. just by printing to a file or to stderr (although I find that hacky and not good enough), caching could not be achieved in this way. So I would really prefer to use the shared context instead, even though it will require more changes to boostrap.
This can be done in one of (probably) two ways:
Build(er). However, one issue with that is bootstrap code actually invokes commands even beforeBuildis created, whenConfigis being parsed. Therefore, we would either need to refactor this code to avoid that (which would be probably quite difficult), or move the shared state to a different struct that is created sooner and it is passed to Config parsing (this is what the third commit does).Build(er)already, and at least being explicit lets us know that the code is accessing an external environment.Let me know what do you think.
r? @onur-ozkan