rustbuild: `x.py fmt src/tools/rustfmt` doesn't format code in the rustfmt subtree

[petrochenkov]

rustfmt's tests require its source code to be properly formatted, but when making changes to rustfmt through its subtree in rust-lang/rust, it seems like there's no way to do such formatting automatically.

@jyn514 suggests that x.py fmt src/tools/rustfmt should just work and do the formatting, and it's probably just a missing step in src/bootstrap/fmt.rs that prevents it from working.

[Mark-Simulacrum]

It's explicitly excluded (like other subtrees), because rustfmt's tests probably are using a different rustfmt to x.py fmt? https://github.com/rust-lang/rust/blob/master/rustfmt.toml#L36

Do you have a pointer to the tests in rustfmt that check it's own source code?

[jyn514]

@Mark-Simulacrum

$ x test src/tools/rustfmt
Mismatch at src/lib.rs:25:
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::fmt;
-use std::io:: {self, Write};
+use std::io::{self, Write};
 use std::mem;
 use std::panic;
 use std::path::PathBuf;
test test::self_tests ... FAILED
test test::system_tests ... ok
test test::idempotence_tests ... ok

failures:

---- test::self_tests stdout ----
Ran 5 self tests.
thread 'test::self_tests' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: 1 self tests failed', src/tools/rustfmt/src/test/mod.rs:357:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test::self_tests

test result: FAILED. 154 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.96s

[Mark-Simulacrum]

Hm. Yeah, so looks like we will probably end up fighting ourselves at some point if we try to format with x.py fmt, but if we don't then this kind of error will also appear. @rust-lang/wg-rustfmt, do you think this test adds a lot of value? It may make sense to just remove it -- the rustfmt source code is probably not terribly interesting from a formatting perspective.

[calebcartwright]

I've mentioned before that I think we might want to consider using the in-tree version of rustfmt to format the rustfmt tree in this repo now that it's a submodule subtree. However, I'm pretty strongly opposed to dropping the test here given that r-l/rust uses an older, pinned version and the bidirectional nature of substree syncs.

If this is dropped and/or rustfmt source is formatted with a different/older version then what happens in r-l/rustfmt then I see it adding undo complexity and increased merge conflicts on those syncs

[calebcartwright]

It may make sense to just remove it -- the rustfmt source code is probably not terribly interesting from a formatting perspective.

that test is also the only viable mechanism we have for enforcing formatting consistency of our own code in the main repo where the development happens on that code, so it's pretty important from that perspective too 😄

[Mark-Simulacrum]

I've mentioned before that I think we might want to consider using the in-tree version of rustfmt to format the rustfmt tree in this repo now that it's a submodule subtree.

That would require everyone building rustfmt locally, right? That doesn't seem feasible.

However, I'm pretty strongly opposed to dropping the test here given that r-l/rust uses an older, pinned version and the bidirectional nature of substree syncs.

I was suggesting we remove the test entirely, and that upstream rustfmt repo check formatting in CI against the same version used locally in rust-lang/rust (perhaps by reading - via GitHub API - src/stage0.json or something; or just pinning and manually bumping from time to time the nightly used).

[jyn514]

That would require everyone building rustfmt locally, right? That doesn't seem feasible.

FWIW it might make sense to format src/tools/rustfmt with itself, but use the beta rustfmt for the rest of the tree.
Another alternative is to turn download-rustc = "if-unchanged" on by default so building rustfmt doesn't take too much time.

[calebcartwright]

FWIW it might make sense to format src/tools/rustfmt with itself, but use the beta rustfmt for the rest of the tree

That's the type of thing I was referring to as well, though not sure how feasible/difficult it would be to have it only run on some explicit invocation/conditional flag.

I was suggesting we remove the test entirely, and that upstream rustfmt repo check formatting in CI against the same version used locally in rust-lang/rust (perhaps by reading - via GitHub API - src/stage0.json or something; or just pinning and manually bumping from time to time the nightly used).

I still feel the same, and am not keen on adding this type of complexity and sacrificing a test that is important to rustfmt.

In general I don't think folks should be making too many changes to subtrees over here, certainly not to the extent that this is a significant problem as opposed to an occasional paper cut.

We'd have a new chunk of work to do on the rustfmt side (some x.py equivalent to ensure folks can locally run the same, older version) and we'd lose the only test that has idempotency checks against an actual codebase. That's quite literally a step in the wrong direction of where I want to take our testing.

[calebcartwright]

I could live with an update to the test that conditionally skips execution, e.g. when RUSTC_BOOTSTRAP is set so that it doesn't impact folks over here.

I would feel even more comfortable about that if we could get the bots to still ping teams if submod/subtree changes are added after the PR opens and/or if we could find another way to be more proactively kept in the loop on subtree changes that get made in this repo

[jyn514]

FWIW it might make sense to format src/tools/rustfmt with itself, but use the beta rustfmt for the rest of the tree

I looked into this and it was trickier than expected. If we build rustfmt unconditionally, it means building the whole compiler on each x.py fmt, which is more of a compile time hit than I'm willing to accept, but if we only build it on changes to rustfmt we have to be considerably smarter about formatting than we are today ...

@Mark-Simulacrum at one point suggested having ~everything be downloadable from CI if it hasn't been modified. I wonder if we could start downloading rustfmt and using stage 2 unconditionally (falling back to building when it's been changed)? Then we naturally get what we want:

• there's a built-in check for @calebcartwright that rustfmt doesn't change its behavior when modified
• we can format the subtree so the rustfmt tests pass for @petrochenkov on changes
• we don't impact compile times for x.py fmt in the common case when rustfmt hasn't been modified
• as a bonus, bootstrap gets to be less special since we no longer have to use a beta version that differs from the rest of the stage0 toolchain. this helps with my long-term plan to have downloads be managed by rustup/RTIM instead of bootstrap.

[Mark-Simulacrum]

we can format the subtree so the rustfmt tests pass for @petrochenkov on changes

I suspect that in many cases this wouldn't be true? rustfmt doesn't dynamically link against the locally compiled code (and probably that's not really possible), so any changes in rustc wouldn't be picked up by rustfmt.

Changes to rustfmt code that rely on rustc would cause a recompile and reformat, but at that point you might as well just build rustfmt (via x.py) and adjust config.toml to point at the built copy temporarily.

In CI, I'm happy to have us have some builder that does this (builds rustfmt and then re-formats the repo, failing if there are any changes), it should be relatively fast to add that to e.g. the tools builder, since it's not doing much work on top of building rustfmt to format things.

This could even be part of the rustfmt test step, for example.