Enable incremental rustfmt adoption #65939

anp · 2019-10-29T16:05:17Z

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:

an [ignore] section to rustfmt.toml including most of the repository
./x.py downloads rustfmt from a specific nightly (we do not pin to beta or stable as we need unstable features)
an ./x.py fmt [--check] command which runs cargo-fmt
./x.py fmt --check runs during the same test step as src/tools/tidy, on master, but not on beta or stable as we don't want to depend on nightly rustfmt there.
a commit to format src/librustc_fs_util as an initial target and to ensure enforcement is working from the start

rust-highfive · 2019-10-29T16:05:27Z

r? @nikomatsakis

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

nikomatsakis · 2019-10-29T18:09:18Z

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?

Mark-Simulacrum · 2019-10-29T18:57:38Z

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.

Mark-Simulacrum · 2019-10-29T23:00:30Z

Would it make more sense to use the bootstrap toolchain's rustfmt binary? What's the best way to experiment with that?

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.

anp · 2019-10-30T02:52:32Z

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:

modify bootstrap+tidy to know how to use x.py's rustfmt as part of its style checking (working on this)
add a documented command somewhere (maybe a bootstrap subcommand?) that can be used to format one's code locally

Centril · 2019-10-30T10:59:04Z

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

Mark-Simulacrum · 2019-10-30T14:57:08Z

Cool! I think I have bootstrap.py fetching the binary (see pushed commit).

Yep, this looks good.

I think we should add it as an x.py command, e.g., x.py fmt to mirror cargo fmt, which would have a --check flag for CI (similar to cargo fmt I believe), it would also know how to run rustfmt/cargo fmt with the appropriate flags and such.

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 impl Step for Tidy (not sure on naming) in src/bootstrap/test.rs).

Mark-Simulacrum · 2019-10-31T02:32:22Z

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.

rust-highfive · 2019-10-31T02:42:26Z

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-10-31T02:24:30.5993660Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-31T02:24:30.6178154Z ##[command]git config gc.auto 0
2019-10-31T02:24:30.6249888Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-31T02:24:30.6320128Z ##[command]git config --get-all http.proxy
2019-10-31T02:24:30.6447553Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65939/merge:refs/remotes/pull/65939/merge

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 @TimNN. (Feature Requests)

anp · 2019-11-01T00:38:20Z

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.

src/bootstrap/format.rs

src/stage0.txt

This was recommended in rust-lang/rust#65939 (comment).

Mark-Simulacrum · 2019-11-04T03:06:40Z

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.

anp · 2019-11-04T15:12:45Z

This makes sense! Another thought: maybe instead of formatting src/bootstrap I should pick an even smaller target like src/build_helper?

anp · 2019-11-04T15:13:49Z

Note to self: this PR should probably include or immediately kick off changes to the rustc guide.

Mark-Simulacrum · 2019-11-04T15:27:00Z

This makes sense! Another thought: maybe instead of formatting src/bootstrap I should pick an even smaller target like src/build_helper?

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).

anp · 2019-11-04T16:30:29Z

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/Cargo.toml

src/bootstrap/lib.rs

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.

Mark-Simulacrum · 2019-12-22T01:24:16Z

Rebased. @bors r+

bors · 2019-12-22T01:24:17Z

📌 Commit dddd872 has been approved by Mark-Simulacrum

Centril · 2019-12-22T01:26:59Z

@bors p=210

bors · 2019-12-22T03:26:45Z

⌛ Testing commit dddd872 with merge 242f3d774c05dca236a07e1564720a37ef6a7c31...

rust-highfive · 2019-12-22T03:42:38Z

The job x86_64-gnu-distcheck of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-22T03:39:51.5560941Z    Compiling toml v0.5.3
2019-12-22T03:39:51.9217012Z    Compiling serde_json v1.0.40
2019-12-22T03:39:55.1377441Z    Compiling bootstrap v0.0.0 (/checkout/obj/build/tmp/distcheck/src/bootstrap)
2019-12-22T03:40:30.4746169Z     Finished dev [unoptimized] target(s) in 1m 23s
2019-12-22T03:40:32.2588099Z thread 'main' panicked at 'std::fs::read_to_string(build.src.join("rustfmt.toml")) failed with No such file or directory (os error 2)', src/bootstrap/format.rs:41:26
2019-12-22T03:40:32.2589770Z failed to run: /checkout/obj/build/tmp/distcheck/build/bootstrap/debug/bootstrap test
2019-12-22T03:40:32.2595711Z Build completed unsuccessfully in 0:01:42
2019-12-22T03:40:32.2596666Z make: *** [check] Error 1
2019-12-22T03:40:32.2596666Z make: *** [check] Error 1
2019-12-22T03:40:32.2597274Z Makefile:48: recipe for target 'check' failed
2019-12-22T03:40:32.2597636Z 
2019-12-22T03:40:32.2597800Z command did not execute successfully: "make" "make" "check"
2019-12-22T03:40:32.2597990Z expected success, got: exit code: 2
2019-12-22T03:40:32.2598122Z 
---
2019-12-22T03:40:32.2599943Z   local time: Sun Dec 22 03:40:31 UTC 2019
2019-12-22T03:40:32.2600168Z   network time: Sun, 22 Dec 2019 03:40:31 GMT
2019-12-22T03:40:32.2600348Z == end clock drift check ==
2019-12-22T03:40:41.8523626Z 
2019-12-22T03:40:41.8642821Z ##[error]Bash exited with code '1'.
2019-12-22T03:40:41.8687929Z ##[section]Starting: Checkout
2019-12-22T03:40:41.8690554Z ==============================================================================
2019-12-22T03:40:41.8690667Z Task         : Get sources
2019-12-22T03:40:41.8690751Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 @TimNN. (Feature Requests)

bors · 2019-12-22T03:42:53Z

💔 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.

Mark-Simulacrum · 2019-12-22T12:45:27Z

@bors r+

Fixed distcheck -- if we don't find rustfmt.toml we'll just bail out of formatting.

bors · 2019-12-22T12:45:29Z

📌 Commit b9e4174 has been approved by Mark-Simulacrum

bors · 2019-12-22T13:34:11Z

⌛ Testing commit b9e4174 with merge 0d2817a...

…crum Enable incremental rustfmt adoption 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: * an `[ignore]` section to `rustfmt.toml` including most of the repository * `./x.py` downloads rustfmt from a specific nightly (we do not pin to beta or stable as we need unstable features) * an `./x.py fmt [--check]` command which runs cargo-fmt * `./x.py fmt --check` runs during the same test step as `src/tools/tidy`, on master, but not on beta or stable as we don't want to depend on nightly rustfmt there. * a commit to format `src/librustc_fs_util` as an initial target and to ensure enforcement is working from the start

bors · 2019-12-22T16:52:47Z

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 0d2817a to master...

eddyb · 2020-03-22T23:41:06Z

src/bootstrap/format.rs

+    let status = cmd.status().expect("executing rustfmt");
+    assert!(status.success(), "running {} successful", cmd_debug);


One rustfmt --check failure should not prevent running rustfmt --check on other files, otherwise you only get one error at a time.

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?

It looks like @Mark-Simulacrum pushed commits to this PR to run rustfmt on individual files.

Aha - maybe file a bug so someone can pick up a fix?

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.

This comment -- "by default we ignore everything in the repository" -- was added in rust-lang#65939 when rustfmt was first being introduced for this repository and (briefly) every directory was ignored. Since then lots of directories have opted in to formatting, so it is no longer true.

rust-highfive assigned nikomatsakis Oct 29, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2019

rust-highfive assigned Mark-Simulacrum and unassigned nikomatsakis Oct 29, 2019

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 29, 2019

anp force-pushed the incremental-rustfmt-rollout branch from 78794d1 to 7a52437 Compare October 30, 2019 02:59

anp changed the title ~~Hypothesis for improving rustfmt adoption within rustc?~~ Incremental rustfmt adoption Oct 31, 2019

anp force-pushed the incremental-rustfmt-rollout branch from 9fdefe3 to d33bd58 Compare November 1, 2019 00:37

Mark-Simulacrum reviewed Nov 1, 2019

View reviewed changes

src/bootstrap/format.rs Outdated Show resolved Hide resolved

src/bootstrap/format.rs Outdated Show resolved Hide resolved

src/bootstrap/format.rs Outdated Show resolved Hide resolved

src/bootstrap/format.rs Outdated Show resolved Hide resolved

src/stage0.txt Outdated Show resolved Hide resolved

anp changed the title ~~Incremental rustfmt adoption~~ Enable incremental rustfmt adoption Nov 2, 2019

anp added a commit to anp/rustfmt that referenced this pull request Nov 2, 2019

Allow RUSTFMT_BOOTSTRAP=1 to override nightly feature checks.

25102cc

This was recommended in rust-lang/rust#65939 (comment).

anp mentioned this pull request Nov 2, 2019

RUSTFMT_BOOTSTRAP=1 allows the compiler's stage0 toolchain to be used upstream rust-lang/rustfmt#3900

Closed

anp force-pushed the incremental-rustfmt-rollout branch from 50e8b75 to b1d0cdf Compare November 4, 2019 16:23

Mark-Simulacrum reviewed Nov 4, 2019

View reviewed changes

src/bootstrap/Cargo.toml Outdated Show resolved Hide resolved

src/bootstrap/lib.rs Outdated Show resolved Hide resolved

anp and others added 3 commits December 21, 2019 20:23

Format src/librustc_fs_util.

5f17b63

In total it's about 100 lines of code and has received less than 5 commits in 2019 -- a good starting point.

Include formatting check in the test step for tidy.

2b081ab

Implement rustfmt running manually using ignore crate

dddd872

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.

Mark-Simulacrum force-pushed the incremental-rustfmt-rollout branch from a8b794e to dddd872 Compare December 22, 2019 01:24

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 22, 2019

Do not run if rustfmt.toml does not exist

b9e4174

distcheck (and generally publishing tarballs) will not package rustfmt.toml and we for now still support running tidy etc in those tarballs.

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 Dec 22, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 22, 2019

bors merged commit b9e4174 into rust-lang:master Dec 22, 2019

anp deleted the incremental-rustfmt-rollout branch December 23, 2019 16:55

semarie mentioned this pull request Dec 26, 2019

unable to build rust on tiers-3 platform #67624

Closed

eddyb reviewed Mar 22, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable incremental rustfmt adoption #65939

Enable incremental rustfmt adoption #65939

anp commented Oct 29, 2019 •

edited by Mark-Simulacrum

Loading

rust-highfive commented Oct 29, 2019

nikomatsakis commented Oct 29, 2019

Mark-Simulacrum commented Oct 29, 2019

Mark-Simulacrum commented Oct 29, 2019

anp commented Oct 30, 2019

Centril commented Oct 30, 2019

Mark-Simulacrum commented Oct 30, 2019

Mark-Simulacrum commented Oct 31, 2019

rust-highfive commented Oct 31, 2019

anp commented Nov 1, 2019

Mark-Simulacrum commented Nov 4, 2019

anp commented Nov 4, 2019

anp commented Nov 4, 2019

Mark-Simulacrum commented Nov 4, 2019

anp commented Nov 4, 2019

Mark-Simulacrum commented Dec 22, 2019

bors commented Dec 22, 2019

Centril commented Dec 22, 2019

bors commented Dec 22, 2019

rust-highfive commented Dec 22, 2019

bors commented Dec 22, 2019

Mark-Simulacrum commented Dec 22, 2019

bors commented Dec 22, 2019

bors commented Dec 22, 2019

bors commented Dec 22, 2019

eddyb Mar 22, 2020 •

edited

Loading

anp Mar 23, 2020

eddyb Mar 23, 2020

anp Mar 23, 2020

Mark-Simulacrum Mar 23, 2020

		let status = cmd.status().expect("executing rustfmt");
		assert!(status.success(), "running {} successful", cmd_debug);

Enable incremental rustfmt adoption #65939

Enable incremental rustfmt adoption #65939

Conversation

anp commented Oct 29, 2019 • edited by Mark-Simulacrum Loading

rust-highfive commented Oct 29, 2019

nikomatsakis commented Oct 29, 2019

Mark-Simulacrum commented Oct 29, 2019

Mark-Simulacrum commented Oct 29, 2019

anp commented Oct 30, 2019

Centril commented Oct 30, 2019

Mark-Simulacrum commented Oct 30, 2019

Mark-Simulacrum commented Oct 31, 2019

rust-highfive commented Oct 31, 2019

anp commented Nov 1, 2019

Mark-Simulacrum commented Nov 4, 2019

anp commented Nov 4, 2019

anp commented Nov 4, 2019

Mark-Simulacrum commented Nov 4, 2019

anp commented Nov 4, 2019

Mark-Simulacrum commented Dec 22, 2019

bors commented Dec 22, 2019

Centril commented Dec 22, 2019

bors commented Dec 22, 2019

rust-highfive commented Dec 22, 2019

bors commented Dec 22, 2019

Mark-Simulacrum commented Dec 22, 2019

bors commented Dec 22, 2019

bors commented Dec 22, 2019

bors commented Dec 22, 2019

eddyb Mar 22, 2020 • edited Loading

Choose a reason for hiding this comment

anp Mar 23, 2020

Choose a reason for hiding this comment

eddyb Mar 23, 2020

Choose a reason for hiding this comment

anp Mar 23, 2020

Choose a reason for hiding this comment

Mark-Simulacrum Mar 23, 2020

Choose a reason for hiding this comment

anp commented Oct 29, 2019 •

edited by Mark-Simulacrum

Loading

eddyb Mar 22, 2020 •

edited

Loading