Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use rustfmt given by RUSTFMT env var #4419

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Use rustfmt given by RUSTFMT env var #4419

merged 1 commit into from
Sep 16, 2020

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Sep 9, 2020

This matches how Cargo uses the rustc and rustdoc specified by RUSTC and RUSTDOC environment variables (see https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-reads).

Trying to do this via PATH instead does not work because Cargo always prepends its own bin directory to PATH before calling any subcommand, and Rustup places its own rustfmt executable there.

$ PATH=/path/to/custom:$PATH cargo fmt  # does not call /path/to/custom/rustfmt

@calebcartwright
Copy link
Member

I'm not opposed to this, but is there any additional context you could share around the use case? Curious if it's a case of wanting to run rustfmt via a different version of cargo fmt or something else.

There would also be some serious compatibility issues in the event someone tried to use v1 of one and v2 of the other

@dtolnay
Copy link
Member Author

dtolnay commented Sep 10, 2020

Sure thing -- my work codebase is a big monorepo (>2 million lines of first-party Rust code) in which:

  • All projects are required to follow a consistent formatting (no custom rustfmt.toml allowed per project).
  • Practically all projects build with Buck. Buck invokes rustc directly. We build rustc from source. There is no rustup or cargo involved.
  • A small number of projects need to do some things using Cargo. For that, the developers have ordinary local rustup setups that are disconnected from the monorepo.
  • The Cargo-using projects want to run cargo fmt -- --check in CI. That is only sensible if their rustup-managed Cargo is able to run the version of rustfmt provided and enforced by the monorepo builds.

In terms of incompatibility between 1.x and 2.x, we are happy to attach an explicit -- --recursive when cargo-fmt is 1.x and rustfmt is 2.x.

@calebcartwright
Copy link
Member

That's really helpful info, thanks!

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@topecongiro - will hold off on merging for a bit in case you have any objections, but seems a reasonable use case to me that we should support. If enough users end up leveraging and getting confused/hitting issues (for example the 1.x vs. 2.x) then we could address with a note to the docs and/or issue template

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

We should add documentation about the RUSTFMT environment variable to our README.md and the cargo's documentation. I will create an issue to track these, and merge this PR first.

@topecongiro topecongiro merged commit 39fe0e1 into rust-lang:master Sep 16, 2020
@calebcartwright
Copy link
Member

I'm going to create a new branch to include the next rustc-ap bump in v1.4.22, and will pull these changes in so that it gets included with the next rustfmt update that hits nightly.

@dtolnay dtolnay deleted the env branch September 17, 2020 02:59
calebcartwright pushed a commit to calebcartwright/rustfmt that referenced this pull request Oct 4, 2020
bors added a commit to rust-lang/cargo that referenced this pull request Oct 13, 2020
Document RUSTFMT environment variable

This PR documents the `RUSTFMT` enviorment variable, as specified in [rust-lang/rustfmt/4426](rust-lang/rustfmt#4426) and [rust-lang/rustfmt/4419](rust-lang/rustfmt#4419).
richardwhiuk added a commit to Metaswitch/swagger-rs that referenced this pull request Jan 12, 2021
RUSTFMT=1 changes the binary used by cargo fmt - see
rust-lang/rustfmt#4419
richardwhiuk added a commit to Metaswitch/swagger-rs that referenced this pull request Jan 12, 2021
RUSTFMT=1 changes the binary used by cargo fmt - see
rust-lang/rustfmt#4419

Signed-off-by: Richard Whitehouse <richard.whitehouse@metaswitch.com>
@karyon
Copy link
Contributor

karyon commented Oct 27, 2021

backported in #4453

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants