RUSTFMT_BOOTSTRAP=1 allows the compiler's stage0 toolchain to be used upstream

[anp]

I'm working on tooling to automate enforcement of rustfmt usage upstream. My current approach requires downloading a nightly rustfmt, while the compiler prefers to use beta toolchain releases for stage0. In a review comment @Mark-Simulacrum recommended adding this environment variable similar to the one used by rustc for bootstrapping.

Is this the right approach? Are there other places this should be propagated?

[Mark-Simulacrum]

I am not sure who "owns" rustfmt these days -- but I would personally feel comfortable landing this change and getting it into beta (via backport or whatever) if necessary to enable us to move forward on the rustc side of things.

cc @nrc, do you perhaps know who to reach out to here?

[calebcartwright]

cc @topecongiro @scampi

[calebcartwright]

Also, just my two cents that the approach/changes look fine but I'm not sure what all the considerations are related to backporting.

For example, master is currently being used for the rustfmt 2.0 release, and there's accordingly some breaking 2.0-specific changes there (like the deprecation of skip_children). As such I'm not sure if this change being made on top of those breaking changes in master will make it more difficult to get this update released/backported so I defer to @topecongiro and @scampi

[Mark-Simulacrum]

It'll probably be best to get the patch targeting a branch (or a new branch, if needed) based off of 1.40 beta once this is ready to land (we're in the process of switching beta right now on rust-lang/rust, so what beta is is a bit murky).

This is also the first I'm hearing about a 2.0 rustfmt, so not sure how much this all ties into that. I suspect we want this in both 1.x and 2.x series, presuming we're expecting future releases of 1.x as well.

[calebcartwright]

presuming we're expecting future releases of 1.x as well

I don't think any more 1.x releases were planned. 1.4.9 or 1.4.10 was to be the last 1.x versions (barring any major hotfixes of course).

2.0 doesn't have a timeline either, though I believe January was mentioned as a loose target

[Mark-Simulacrum]

Hm, so to be clear I'm viewing this from a Rust distribution perspective (AFAIK, we don't expect users to be installing rustfmt themselves), so it's not entirely clear to me how a 2.0 would fit into that with regards to our general stability policies -- I'm hopeful that this is explained somewhere but not sure where to look -- if I follow https://github.com/rust-lang/rustfmt/blob/master/Contributing.md it's an edition-like change where users would opt-in to new formatting, so that then means that rustfmt itself never breaks on an upgrade, right?

In any case sounds like we'd want a 1.x release done with this patch in it.

Basically, I am surprised by this:

$ rustfmt --version
rustfmt 1.4.9-nightly (33e36670 2019-10-07)

I would expect something like how RLS looks, which is "rls 1.39.0 (80a1d34 2019-09-20)" -- i.e., the Rust version number is the version number.

[calebcartwright]

Apologies if I'm causing any confusion 😄 I'm only speaking to things from the rustfmt perspective, and even so, I've no idea how rustfmt flows into the main Rust distributions outside of the first part of the process in the release documentation for rustfmt

In any case sounds like we'd want a 1.x release done with this patch in it.

I believe so yes, and that's what was behind my thinking around the 1.x vs. 2.x versions of rustfmt.

My assumption is that this feature is needed soon (at least far sooner than a rustfmt 2.0 release). So I was thinking it'd probably be easier on the rustfmt side to create a new branch (presumably based off the 1.4.9 release), and then make these updates on top of that branch to make it easier to publish a 1.4.10 (or some other 1.x) version of rustfmt with this change.

[anp]

It's probably worth noting that one reason we need this environment variable is to support the version = "Two" directive that's currently in the repository. Is this versioning related to rustfmt 1.0/2.0?

The other is for use of the top-level [ignore] table.

[Mark-Simulacrum]

The branching sounds like the correct model; we'll want to land this patch onto beta (so, based on 33e3667, that is the 1.4.9 tag, it looks like) and that probably means a 1.4.10 release.

(It feels like we may never want rustfmt 2.x as a version, since that implies a breaking change, rather we'd maybe change defaults along edition lines or so? That's a discussion for a separate thread though :)

[calebcartwright]

Is this versioning related to rustfmt 1.0/2.0?

Yup! Those are version gates used to ensure the default formatting is the same for a major version of rustfmt, with the ability to opt-in to the formatting style of the next major version. It's explained in better detail here

The other is for use of the top-level [ignore] table.

Right and ignore is still a nightly-only rustfmt option that you need to be able to use on beta, and this change will allow you to be able to use ignore on beta, correct?

[calebcartwright]

(It feels like we may never want rustfmt 2.x as a version, since that implies a breaking change, rather we'd maybe change defaults along edition lines or so? That's a discussion for a separate thread though :)

For reference, some issues with historical discussions around rustfmt versioning strategy:
#612
rust-lang/rfcs#2437
#2614
#2877

[topecongiro]

Sorry for the late reply, I was away from the keyboard this weekend.

@anp

It's probably worth noting that one reason we need this environment variable is to support the version = "Two" directive that's currently in the repository. Is this versioning related to rustfmt 1.0/2.0?

Does this mean that if we stabilize ignore and version configuration options then we do not need this env var?

@Mark-Simulacrum

I would expect something like how RLS looks, which is "rls 1.39.0 (80a1d34 2019-09-20)" -- i.e., the Rust version number is the version number.

AFAIK that only applies to rls. Other 'official' tools like rustup, rustfmt and clippy have their own versioning schema.

[Mark-Simulacrum]

Does this mean that if we stabilize ignore and version configuration options then we do not need this env var?

I think this is probably true, at least for now. But I suspect it'll be worthwhile to dogfood future unstable features in the compiler tree, right? If we don't think that's the case then we can definitely "just" stabilize instead of this, but I suspect the timeline on stabilization is a bit longer than we'd want, too.

[topecongiro]

I think this is probably true, at least for now. But I suspect it'll be worthwhile to dogfood future unstable features in the compiler tree, right? If we don't think that's the case then we can definitely "just" stabilize instead of this, but I suspect the timeline on stabilization is a bit longer than we'd want, too.

I agree that using unstable features in the compiler tree is completely acceptable but then why can't we just use nightly rustfmt? Is there any reason to prefer beta over nightly?

[anp]

I believe the usage of a beta toolchain for stage0 bootstrap is for a bunch of reasons but @Mark-Simulacrum should fill in there.

Does this mean that if we stabilize ignore and version configuration options then we do not need this env var?

Is it possible for an unstable rustfmt feature to be required in order to format new syntax elements?

[Mark-Simulacrum]

I agree that using unstable features in the compiler tree is completely acceptable but then why can't we just use nightly rustfmt? Is there any reason to prefer beta over nightly?

The reason essentially comes down to a couple things. We want a "known" time to switch onto the next version of rustfmt, and beta switching is a good place for that. We also currently always want to be able to bootstrap off of the last beta.

However, the more I think about this, we might not actually want this, especially around adding new syntax and such (which presumably makes rustfmt fall down) -- and since presumably we could only run this on master, not beta/stable branches, we could possibly get away with just having rustfmt be a nightly version for now at least.

I would like to land this regardless, though, since I suspect we'll possibly still want it (at least in the future). I'll followup per my previous paragraph to try and move us forward on rust-lang/rust and try and reduce the pressure a little here hopefully :)

[anp]

I would like to land this regardless, though, since I suspect we'll possibly still want it (at least in the future).

If we're not going to use the environment variable in rust-lang/rust yet then we should probably have a test, right? Given the small size of the patch I'm probably more inclined to close/postpone this until that need actually arises. Thoughts?

[topecongiro]

@Mark-Simulacrum @anp As a maintainer of rustfmt, I truly appreciate the work of easing the adoption of rustfmt to the rustc repo :)

That being said, rustc is still just one of users of rustfmt (though admittedly the special one). I am not in favor of adding a feature to rustfmt that exists just to keep the rustc repo's build step tidy, especially when it's potentially accessible to other users.

If we're not going to use the environment variable in rust-lang/rust yet then we should probably have a test, right? Given the small size of the patch I'm probably more inclined to close/postpone this until that need actually arises. Thoughts?

I would like to close this and reopen/consider merging this only if it's absolutely necessary.

[Mark-Simulacrum]

Okay, I'm going to close this. I suspect we'll actually want this feature in the long term -- it's something I think most Rust tools that gate unstable features will want. But I'm fine with holding off on this until we have an actual need.

[ojeda]

It would be nice to have rustfmt respect RUSTC_BOOTSTRAP or similar.