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

feat(build): Use RUSTFMT to find rustfmt binary #566

Merged
merged 1 commit into from
Mar 11, 2021
Merged

feat(build): Use RUSTFMT to find rustfmt binary #566

merged 1 commit into from
Mar 11, 2021

Conversation

UebelAndre
Copy link
Contributor

@UebelAndre UebelAndre commented Feb 21, 2021

Motivation

The Rust Bazel rules (rules_rust) is looking to add some tools for building targets with Tonic. In development of bazelbuild/rules_rust#479 it was found that tonic-build calls rustfmt and expects this to be in the user's PATH. This is generally not going to be the case when Building in Bazel. We're looking for a way to specify this binary without needing to rely on or modify PATH.

Solution

Cargo already has a pattern for specifying environment variables to control what binaries are used during builds. The change here adopts the same functionality as Cargo and allows for an environment variable (RUSTFMT) to optionally specify the location of the rustfmt binary.

@UebelAndre
Copy link
Contributor Author

@LucioFranco Hey, sorry to ping but do you have a moment to take a look at this? 🙏

@UebelAndre
Copy link
Contributor Author

Hey @LucioFranco, just pining this PR one more time in hopes you have time for a quick review 😅 🙏

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

This looks fine in principle, is that the most common environment variable to check for rustfmt?

@LucioFranco LucioFranco changed the title Allow tonic-build to find rustfmt via a RUSTFMT environment variable feat(build): Use RUSTFMT to find rustfmt binary Mar 11, 2021
@LucioFranco LucioFranco merged commit ea56e2e into hyperium:master Mar 11, 2021
@UebelAndre
Copy link
Contributor Author

Hey @LucioFranco, just pining this PR one more time in hopes you have time for a quick review 😅 🙏

It's the same thing Cargo uses: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-reads

That was my primary goal here, was to have parity with cargo.

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

Successfully merging this pull request may close these issues.

3 participants