Enable incremental rustfmt adoption#65939
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Big 👍 to this approach overall, I have no idea about the question of integrating into tidy though -- maybe we can somehow find it in path? |
|
I'm going to r? @Mark-Simulacrum here and try and find some time this week to get back to you here on the overall approach and such. I'm sure we'll want sign off from T-compiler (and perhaps T-libs) as the primary "owners" of the code in this repository but for now we can likely figure out a game plan to bring to fcp. |
I suspect that the best thing is indeed to use the bootstrap binary, as that'll be a preset way that'll change at the right points and is guaranteed to exist and such. The way to do this today is probably to add another artifact to the things we download (in particular, in src/bootstrap/bootstrap.py). Let me know if you'd like further details on how to do that, I can look into it, though I've not recently explored that piece of code. I agree with @nikomatsakis that this is the right approach, seems like the best way to phase this in. If we want to test this out on a particular subdirectory I think src/bootstrap is perhaps a good candidate as it's fairly small and changes not too often. However, we can also land this I think without enabling it and then enable it for a specific directory in a separate PR. |
|
Cool! I think I have bootstrap.py fetching the binary (see pushed commit). Next step is to figure out the places to run it. I see two:
|
78794d1 to
7a52437
Compare
|
I believe the compiler team already approved of formatting the repo in rust-lang/compiler-team#80. @anp take a look at Centril/rfcs#21 |
Yep, this looks good. I think we should add it as an x.py command, e.g., I think tidy is probably not quite the right place as at least I know that it's sort of not the thing you run by default, i.e., x.py fmt should be runnable "on save" or so from editors I hope whereas tidy is possibly too slow or not a good fit for that. The tidy test step in bootstrap might want to run the fmt check as well though (not the tidy binary, but the |
|
Last two commits look a little different from what I had expected but actually seem like a better approach, so continue on! We should try to keep the PR description up to date with a summary of what we've decided on so far (it'll go into commit history, so treat it as such). Just wanted to write that so we can avoid a surprise of work at the end, no need necessarily to do incremental work to get there, I think it's primarily useful for historical reasons. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
9fdefe3 to
d33bd58
Compare
|
In getting this to work, I found that rustfmt hangs on stdin activity if no paths are passed, so I took @Mark-Simulacrum's recommendation to format src/bootstrap as a starter. |
This was recommended in rust-lang/rust#65939 (comment).
|
One thought -- could we split the bootstrap re-formatting into a separate PR which I can r+ immediately? That way we keep the diff line count in this PR down (making it a little easier to review). I would also like us to try and prepare a (potentially hacky) script that re-formats the repository automatically during rebasing or so with the idea being that a PR that is currently in flight to e.g. bootstrap would not be too hard to rebase atop this. Not sure how easy that is, if it proves pretty much impossible then no need to spend too much time on it. |
|
This makes sense! Another thought: maybe instead of formatting |
|
Note to self: this PR should probably include or immediately kick off changes to the rustc guide. |
Sure, yeah, or maybe even src/libarena -- basically any small and rarely changed crate seems like a fine fit, optimizing a little more for smallness (I had expected src/bootstrap to have a smaller diff, somehow). |
50e8b75 to
b1d0cdf
Compare
|
I picked librustc_fs_util (it's really tiny!) and pushed it to this branch since it's such a low risk for churn and hopefully doesn't increase review burden. Happy to split it up if you'd like though, lmk. |
src/bootstrap/format.rs
Outdated
There was a problem hiding this comment.
Should the edition flag move to rustfmt.toml?
There was a problem hiding this comment.
All of these latest commits are very much just testing things -- I wouldn't expect them to stick around.
24d7934 to
1b9ac08
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
This PR is tentatively blocked on rust-lang/rustfmt#3930 at this point unless someone comes up with a scheme to avoid that. I think it might be possible to do something like filtering based on the ignore list ourselves and running rustfmt with |
|
Oh my! Well that’s a fun discovery. Thank you for your thoroughness while I’m in between desk setups <3. |
e3948a3 to
a8b794e
Compare
|
Okay, I've manually implemented scanning I've dropped the mingw CI builder and rebased to drop all of the debugging commits out. I believe this is ready to land, so I'm going to approve it and try to get it in fairly quickly so that we can then land a fairly large reformatting (to be generated and approved by well-known contributors). |
|
@bors r+ |
|
📌 Commit a8b794efbb251928f6310bcf27d4fa77bec57ba5 has been approved by |
Co-Authored-By: Mark Rousskov <mark.simulacrum@gmail.com>
Co-Authored-By: Mark Rousskov <mark.simulacrum@gmail.com>
In total it's about 100 lines of code and has received less than 5 commits in 2019 -- a good starting point.
This replaces cargo-fmt with rustfmt with --skip-children which should allow us to format code without running into rust-lang/rustfmt#3930. This also bumps up the version of rustfmt used to a more recent one.
|
Rebased. @bors r+ |
|
📌 Commit dddd872 has been approved by |
|
@bors p=210 |
|
⌛ Testing commit dddd872 with merge 242f3d774c05dca236a07e1564720a37ef6a7c31... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
distcheck (and generally publishing tarballs) will not package rustfmt.toml and we for now still support running tidy etc in those tarballs.
|
@bors r+ Fixed distcheck -- if we don't find rustfmt.toml we'll just bail out of formatting. |
|
📌 Commit b9e4174 has been approved by |
|
☀️ Test successful - checks-azure |
| let status = cmd.status().expect("executing rustfmt"); | ||
| assert!(status.success(), "running {} successful", cmd_debug); |
There was a problem hiding this comment.
One rustfmt --check failure should not prevent running rustfmt --check on other files, otherwise you only get one error at a time.
There was a problem hiding this comment.
When that bit was originally written we were running rustfmt once for the whole repository and it showed all errors. Is that no longer how it's running?
There was a problem hiding this comment.
It looks like @Mark-Simulacrum pushed commits to this PR to run rustfmt on individual files.
There was a problem hiding this comment.
Aha - maybe file a bug so someone can pick up a fix?
There was a problem hiding this comment.
Yeah, rustfmt on the whole repository was leading to errors in rustfmt (IIRC, it had some problems with ignoring files or so, I don't recall the specifics now).
This should be fixable though, like we do in tidy etc by keeping track of whether we errored and only stopping at the end.
Enables an incremental rollout of rustfmt usage within the compiler via a granular ignore configuration and automated enforcement. The decision to format the repository was approved in rust-lang/compiler-team#80 (comment).
This PR includes:
[ignore]section torustfmt.tomlincluding most of the repository./x.pydownloads rustfmt from a specific nightly (we do not pin to beta or stable as we need unstable features)./x.py fmt [--check]command which runs cargo-fmt./x.py fmt --checkruns during the same test step assrc/tools/tidy, on master, but not on beta or stable as we don't want to depend on nightly rustfmt there.src/librustc_fs_utilas an initial target and to ensure enforcement is working from the start