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

Add a --build-dir flag to rustbuild #98745

Merged
merged 1 commit into from
Jul 1, 2022
Merged

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Jun 30, 2022

This adds an optional --build-dir <path> flag to rustbuild (to both the python and rust code in src/bootstrap). If provided, it overrides build directory from the config file (if any was provided).

My reason for wanting this is that I often will make a change, save, and then go run x.py check or x.py test (or something). Because I've saved, vscode will start doing its thing in the background, but this will take the file lock, preventing x.py from running until vscode finishes whatever it's doing (since the manually invoked x.py won't be able to acquire said file lock). This is annoying, because I'd rather the command I explicitly invoke not wait for r-a to complete, as r-a's check is conceptually a background task (and one which can take quite some time to complete).

Anyway, while there are likely other ways this could be handled, if you have the disk space an easy way is to just have vscode be configured to use a different build directory, and then they never have to block each-other.

This can currently be arranged without this patch, by maintaining two config.tomls, one of which has a different build dir, and just exists to be passed into the overridden check command in vscode.

Unfortunately, this has the downside of requiring I maintain two config.tomls and keep them (at least somewhat) in sync, aside from the build dir. I dislike for several reasons, not the least of which because I know myself well enough to know that these will inevitably get out of sync and confuse me in the future (perhaps this case would be different since I've thought about it enough to write this patch? Who knows, I'd rather not find out).

Either way, it would be much easier for me to have a way for only the build directory to differ, which this patch provides by way of a new flag. I suggested this to @jyn514 who indicated it sounded reasonable so long as it didn't add too much complexity, which I think I've achieved, but he can be the judge.

Anyway, with this patch I can just use something like ["python3", "x.py", "check", "--build-dir", "build-vscode", "--json-output"] as the overridden check command to rust-analyzer, and do not need to futz with any additional config.tomls. Which is very nice!

I've tested this manually, and can confirm that it works. I'm not sure if it needs automated tests, or where I should add them if so.

r? @jyn514 (who has had to put up with my complaints about this... many times. <3)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2022
@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jun 30, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Thanks for your detailed writeup! The nice thing about doing it this way is --build-dir vscode will work for everyone :) and we can start recommending that in the dev-guide for people who have enough disk space.

@Mark-Simulacrum this is simple to revert so I'm going to go ahead and unilaterally approve - feel free to leave a comment if you think we should avoid doing this for some reason.

src/bootstrap/config.rs Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Jun 30, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 30, 2022

📌 Commit 79f8dc0 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 30, 2022
@Mark-Simulacrum
Copy link
Member

This seems probably okay - I don't have any direct objections. I personally do this by modifying the current directory in the rust-analyzer command and passing --config to a config.toml, which works fine today already. But this seems a little simpler, and probably slightly easier to integrate cross platform. So I think this is probably good.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 1, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#98610 (fix `emit_inference_failure_err` ICE)
 - rust-lang#98640 (Let rust-analyzer ship on stable, non-preview)
 - rust-lang#98686 (add ice test for 46511)
 - rust-lang#98727 (rustdoc: filter '_ lifetimes from ty::PolyTraitRef)
 - rust-lang#98729 (clarify that ExactSizeIterator::len returns the remaining length)
 - rust-lang#98733 (Request to be notified of MIR changes)
 - rust-lang#98734 (Update RELEASES.md)
 - rust-lang#98745 (Add a `--build-dir` flag to rustbuild)
 - rust-lang#98749 (Add macro_rules! rustdoc change to 1.62 relnotes)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 335e7d3 into rust-lang:master Jul 1, 2022
@cuviper cuviper added this to the 1.64.0 milestone Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants