Stream: t-compiler

Topic: x.py fmt


centril (May 03 2019 at 15:19, on Zulip):

@nikomatsakis I noted before that I'd like to get working on a T-Infra+ T-Compiler proposal for our policy re. formatting in the repo; My current idea is something like this:

1. Our goal is:
+ ./x.py fmt will format everything in the repo according to our wants.
+ ./x.py test src/tools/tidy + CI will enforce formatting.
+ @bors fmt will add a commit to the PR where ./x.py fmt has been executed.

2. Rollout plan:
a. We will start by encoding the formatting rules into tidy.
b. tidy will only require formatting inside directories with a .enforcefmt file.
c. We will incrementally start adding .enforcefmt files to higher and higher level folders until there's just one top-level .enforcefmt file.
d. We remove the .enforcefmt file and tidy now enforces it globally everywhere (possible caveat around submodules).

What do you think? Also, what is the best avenue for discuss the draft before publishing the RFC to rust-lang/rfcs?

centril (May 03 2019 at 15:21, on Zulip):

cc @Pietro Albini --^ also, can we technically add commits to a PR with @bors fmt?

Pietro Albini (May 03 2019 at 15:21, on Zulip):

I'd prefer for the automatic command not to be in bors

centril (May 03 2019 at 15:22, on Zulip):

@Pietro Albini any specific reason (so I can include it in the RFC)?

Pietro Albini (May 03 2019 at 15:22, on Zulip):

bors is already pretty fragile and I'd prefer for it to be free of repo-specific commands

Pietro Albini (May 03 2019 at 15:23, on Zulip):

I'm not opposed at all to an automated command though, but maybe triagebot would be better

centril (May 03 2019 at 15:23, on Zulip):

@Pietro Albini yeah; good rationale :slight_smile:

centril (May 03 2019 at 15:23, on Zulip):

triagebot sounds good since it is centralized as opposed to having too many bots.

Pietro Albini (May 03 2019 at 15:24, on Zulip):

(also almost every time we redeploy bors the queue messes up, so it's better to avoid as much changes as possible)

matklad (May 03 2019 at 15:24, on Zulip):

I'd also add ./x.py install-format-hook, which installs pre-commit git-hook for formatting.

Pietro Albini (May 03 2019 at 15:25, on Zulip):

btw, I don't think t-infra should be involved in this as a whole

centril (May 03 2019 at 15:25, on Zulip):

@matklad yeah good idea; we should probably have a script for the rebase fallout

centril (May 03 2019 at 15:26, on Zulip):

@Pietro Albini oh? curious... I'd expect y'all to be involved in vetting the implementation?

Pietro Albini (May 03 2019 at 15:26, on Zulip):

we want infra not to be the team developing bots (for example triagebot is technically under t-release), and the rustbuild stuff is mostly mark and alex

centril (May 03 2019 at 15:26, on Zulip):

@Pietro Albini ah; good point -- rustbuild is also kennytm no?

centril (May 03 2019 at 15:27, on Zulip):

@Pietro Albini So just T-compiler then?

Pietro Albini (May 03 2019 at 15:27, on Zulip):

maybe? not sure how much kenny works on rustbuild

matklad (May 03 2019 at 15:27, on Zulip):

Also, do we already have rustfmt.toml we want to use for the repo? I am writing new code for the lexer, and, if I could, I'd like to format it in a good way from the start

Pietro Albini (May 03 2019 at 15:28, on Zulip):

maybe chime in the next infra meeting and announce y'all are working on this so the interested members can get into the loop

centril (May 03 2019 at 15:29, on Zulip):

@matklad doesn't seem so; I would just start with the standard format and tweak as needed

matklad (May 03 2019 at 15:29, on Zulip):

There was a pretty strong desire to use at least use_small_heuristics = "Max"

centril (May 03 2019 at 15:29, on Zulip):

Ideally, rust-lang/rust shouldn't have too divergent of a formatting from the standard

centril (May 03 2019 at 15:30, on Zulip):

(https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#use_small_heuristics)

centril (May 03 2019 at 15:31, on Zulip):

@matklad sure, that seems good

matklad (May 03 2019 at 15:34, on Zulip):

So, as a first step, let's maybe commit just rustfmt.toml, without any CI enforcement? That'll help with writing new code in a good style immediately

Enforcement is 100% required, but it'll be a long time before we will set up all the bots..

Otoh, reformatting once is not a big deal, so it's a small win anyway. Otth, starting without enforcement should help us to iterate and agree on the style more flexibly.

centril (May 03 2019 at 15:36, on Zulip):

@matklad hopefully it shouldn't take too much time to set this up; just a few E-easy issues :D

matklad (May 03 2019 at 15:36, on Zulip):

also, cc @Vadim Petrochenkov , who is not quite satisfied with default rustfmt style as well

centril (May 03 2019 at 15:37, on Zulip):

@matklad I believe @Vadim Petrochenkov's primary dissatisfaction was due to use_small_heuristics; setting it to Max should be good?

matklad (May 03 2019 at 15:41, on Zulip):

I don't remmeber, better to ask or to dig that internals thread

Vadim Petrochenkov (May 03 2019 at 18:26, on Zulip):

Relevant thread https://internals.rust-lang.org/t/running-rustfmt-on-rust-lang-rust-and-other-rust-lang-repositories/8732/81

Vadim Petrochenkov (May 03 2019 at 18:26, on Zulip):

Setting use_small_heuristics = "Max" and using "rustfmt 2.0 mode" (enables breaking bugfixes) resolved all my concerns.

nikomatsakis (May 03 2019 at 18:32, on Zulip):

Also, do we already have rustfmt.toml we want to use for the repo? I am writing new code for the lexer, and, if I could, I'd like to format it in a good way from the start

Hmm. I feel somewhat strongly we should use the defaults. But I guess I would accept use_small_heuristics = Max, particularly if it means we get rustfmt :)

nikomatsakis (May 03 2019 at 18:32, on Zulip):

I'd be opposed to a growing set of tweaks though.

nikomatsakis (May 03 2019 at 18:32, on Zulip):

Anyway, all of this sounds very good to me. =)

nikomatsakis (May 03 2019 at 18:33, on Zulip):

Hmm. I feel somewhat strongly we should use the defaults.

( To be clear, this has nothing to do with the formatting itself, and everything to do with being simple and uniform. Anyway, I'm good with the compromise. =) )

centril (May 03 2019 at 18:46, on Zulip):

@nikomatsakis OK; I'll work up a draft then and make a PR against the master of my fork for y'all to review

nikomatsakis (May 03 2019 at 18:48, on Zulip):

@centril sounds good -- note that we wound up with a process that uses issues to track meeting proposals; though I think it's fine to prep in a fork and file an issue with a link over there

nikomatsakis (May 03 2019 at 18:48, on Zulip):

I guess I didn't think it all through, in that there is a template... well... whatever

matklad (May 03 2019 at 18:48, on Zulip):

Filed https://github.com/rust-lang/rust/pull/60520 just now :D

nikomatsakis (May 03 2019 at 18:48, on Zulip):

do whatever :)

centril (May 03 2019 at 18:50, on Zulip):

@nikomatsakis aight; cool

@matklad neat

centril (May 05 2019 at 11:35, on Zulip):

@nikomatsakis here you go: https://github.com/rust-lang/compiler-team/issues/80 (draft to RFC included there)

matklad (Aug 19 2019 at 16:34, on Zulip):

@centril what's the current status here? Am I correct that the plan is agreed upon, but ./x.py fmt and .enforcefmt are not implemented yet? Is there a tracking issue?

centril (Aug 19 2019 at 16:35, on Zulip):

@matklad I need to finish up my RFC and publish it

matklad (Aug 19 2019 at 16:36, on Zulip):

Hm, I thought that the agreement has been already reached? https://github.com/rust-lang/compiler-team/issues/80#issuecomment-491324076

centril (Aug 19 2019 at 16:37, on Zulip):

@matklad I believe the specifics still warrants the RFC

nikomatsakis (Aug 19 2019 at 17:47, on Zulip):

Ha, I was just going to ask the exact same question

nikomatsakis (Aug 19 2019 at 17:47, on Zulip):

("what is status")

centril (Aug 19 2019 at 17:48, on Zulip):

@nikomatsakis do you think I should file the RFC or perhaps just a tracking issue is sufficient?

nikomatsakis (Aug 19 2019 at 17:51, on Zulip):

I feel like this probably doesn't require an RFC

nikomatsakis (Aug 19 2019 at 17:51, on Zulip):

It seems like we had reached consensus on compiler team

nikomatsakis (Aug 19 2019 at 17:51, on Zulip):

I dont' think an RFC hurts per se, but it's more overhead for sure

centril (Aug 19 2019 at 17:52, on Zulip):

Alright; I'll think about what requires least effort on my part then / what I can do more quickly

simulacrum (Aug 19 2019 at 17:56, on Zulip):

I personally think the lion share's of the work is figuring out a way to land this smoothly in-tree, but I think mostly x.py fmt is not _too_ hard a change to make (modulo rebasing everyone's PR :)

centril (Aug 19 2019 at 18:00, on Zulip):

It's mostly a one-time cost

centril (Aug 19 2019 at 18:02, on Zulip):

and I believe circulation in rust-lang/rust is fast enough that rebasing can largely be left up to folks to deal with without a script

centril (Aug 19 2019 at 18:06, on Zulip):

@simulacrum should we just go ahead and format the whole repository for now without enforcement?

centril (Aug 19 2019 at 18:06, on Zulip):

queue and total # prs is low

simulacrum (Aug 19 2019 at 18:07, on Zulip):

I personally think that would be helpful but I'd probably move more gradually, e.g. first land a PR that makes rustfmt be part of the downloaded stage0 bundle, and invocable via x.py fmt on any subtree

centril (Aug 19 2019 at 18:07, on Zulip):

Fair idea

simulacrum (Aug 19 2019 at 18:08, on Zulip):

and then as a followup use that infrastructure for formatting and enforcement (in that order, I don't know that it matters if it happens simultaneously. Could be good to split up or combine, I could see both options having their own benefits).

centril (Aug 19 2019 at 18:08, on Zulip):

@simulacrum would you like to file a tracking issue and such? you seem to have a better idea of the implementation

simulacrum (Aug 19 2019 at 18:08, on Zulip):

ah... I'd rather not, no. I probably don't have the will/energy to drive this through myself though I can help review the bootstrap changes

centril (Aug 19 2019 at 18:09, on Zulip):

ok, fair; I'll see when I have time to do so although @matklad might be interested to spearhead this?

mark-i-m (Aug 19 2019 at 18:09, on Zulip):

Can we enforce it on all new PRs? i.e. only for files touched?

mark-i-m (Aug 19 2019 at 18:09, on Zulip):

Then we can incrementally update the rest

centril (Aug 19 2019 at 18:09, on Zulip):

how do we do that technically?

mark-i-m (Aug 19 2019 at 18:09, on Zulip):

We can get the list of changed files from git

simulacrum (Aug 19 2019 at 18:09, on Zulip):

I am fairly strongly against any sort of incremental thing personally since I wouldn't want to have to deal with reverting changes made on save if I enable rustfmt-on-save locally to unrelated PRs

simulacrum (Aug 19 2019 at 18:10, on Zulip):

er, unrelated files

centril (Aug 19 2019 at 18:10, on Zulip):

yea; and it seems like it will mess with history more

simulacrum (Aug 19 2019 at 18:10, on Zulip):

I fairly often save on "accident" so to speak with just muscle memory

mark-i-m (Aug 19 2019 at 18:10, on Zulip):

Why would you need to revert them?

centril (Aug 19 2019 at 18:10, on Zulip):

doing it once seems better

simulacrum (Aug 19 2019 at 18:11, on Zulip):

I'd prefer that formatting commits are only formatting commits -- I think we can afford to do it either by-crate or even just a global single PR, authored and landed by someone we trust

simulacrum (Aug 19 2019 at 18:11, on Zulip):

(Since reviewing it is impossible)

mark-i-m (Aug 19 2019 at 18:11, on Zulip):

fair enough

centril (Aug 19 2019 at 18:11, on Zulip):

@simulacrum we could split the formatting of the code folders and the test folders

simulacrum (Aug 19 2019 at 18:12, on Zulip):

Sure. I don't care much, just separate PRs, not rolled into normal ones.

centril (Aug 19 2019 at 18:12, on Zulip):

but ultimately trust is necessary

centril (Aug 19 2019 at 18:12, on Zulip):

of course

Vadim Petrochenkov (Aug 19 2019 at 20:21, on Zulip):

Was the issue of rustfmt breaking 100-character limit and other tidy checks resolved, btw?

centril (Aug 19 2019 at 20:51, on Zulip):

Not sure; we'll find out quickly tho if tidy complains :slight_smile:

Last update: Nov 21 2019 at 14:35UTC