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

WIP: move the 'record all toolstates' list into rustbuild #74389

Closed
wants to merge 1 commit into from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 16, 2020

Currently, we still build Miri on beta/stable branches to record its toolstate. That seems rather unnecessary. This is a beginning of an attempt to fix that.

However, I think I have to give up here... I have no idea if this execute_cli_for_paths makes any sense; the control flow in rustbuild is entirely impossible to follow.^^ For some reason --dry-run does not work for my new target (it does nothing). Maybe that is because of this execute_cli; I tried to understand the existing dry_run logic but it is very strange (if this is not a dry run then the first thing we do is set dry_run to true?!??) and there are no comments explaining what happens.

I am pretty sure what I have so far is wrong; the code in toolstate.rs all assumes that all tools have a state recorded in the JSON:

for (tool, _) in STABLE_TOOLS.iter().chain(NIGHTLY_TOOLS.iter()) {

and this loop looks like it makes similar assumptions:

for (tool, submodule) in STABLE_TOOLS.iter().chain(NIGHTLY_TOOLS.iter()) {

@Mark-Simulacrum I think someone who actually understands the maze that is rustbuild needs to complete this. It would take me forever to figure this all out, I have no idea how to test any of this with reasonable effort, and it's just not enough of a problem I think to warrant that amount of effort.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2020
@RalfJung RalfJung mentioned this pull request Jul 16, 2020
@@ -5,22 +5,14 @@ set -eu
X_PY="$1"

# Try to test all the tools and store the build/test success in the TOOLSTATE_FILE

set +e
Copy link
Member Author

Choose a reason for hiding this comment

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

This line should also have a comment (I can just guess what happens here).

@RalfJung
Copy link
Member Author

Closing this as I do not plan to continue working on it; opened #74709 to track the underlying problem.

@RalfJung RalfJung closed this Jul 24, 2020
@RalfJung RalfJung deleted the toolstate branch March 27, 2021 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants