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

Pull/expose rustfmt binaries #291

Merged
merged 5 commits into from
Mar 9, 2020
Merged

Conversation

colatkinson
Copy link
Contributor

See #87.

  • Uses load_arbitrary_tool() to download rustfmt from its independent package (so no need to pull in the full rust archive).
    • Can be invoked via bazel run @rust_linux_x86_64//:rustfmt -- --check $PWD/greeter.rs.
    • TODO: Is there a way to expose this via a platform-independent workspace?
  • Retrieves the rustfmt hashes in fetch_shas.sh.
    • This is currently pretty ugly due to rustfmt and rustc being independently versioned.
  • Sets LC_ALL=C in fetch_shas.sh.
    • This was actually causing a different sort order for the hashes on my machine. Probably my fault for not correctly configuring my locales. IMO seems like a quick way to avoid pain for other contributors. Can easily be cherry-picked off, though.

Prebuilt binaries don't seem to be available for FreeBSD, so unfortunately the tool won't be available on that platform.

TODO: What's the easiest way to write tests for this?
TODO: Can the changes to fetch_shas.sh be made nicer?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@colatkinson
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@mfarrugi
Copy link
Collaborator

mfarrugi commented Jan 16, 2020

TODO: Is there a way to expose this via a platform-independent workspace?

I know @bazel_tools//tools/cpp:current_cc_toolchain exists, but it looks like the aliasing/magic is implemented in Java.

TODO: What's the easiest way to write tests for this?

You could add a formatting step to one of the codegen rules (ie. protobuf), or write a genrule that invokes rustfmt.

It probably makes sense to make rustfmt available on rust_toolchain as part of this so that bindgen, protobuf and any future codegen could make use of it programmatically as well. I think this also simplifies usage from genrules, but not as sure on that.

Edit: A more drop-in place to get testing would be to change bindgen.bzl#L54 so that rustfmt_bin falls back to the rust_toolchain rustfmt instead of its own (which was done as a bit of a hack at the time).

@colatkinson
Copy link
Contributor Author

Couple of changes:

  • rustfmt binary is now exposed via the Rust toolchain.
  • As suggested, the bindgen rules now fall back on the provided rustfmt if available. Tested locally, and it appears to work as intended.
  • There's also an explicit test for the binary. A sample rule generates a formatted file from an unformatted one, and diff is used to verify that the files differ. Not very comprehensive, so any suggestions on how to beef it up are appreciated.
    • I believe this will fail on BSD, but I haven't yet set up a test environment yet. I haven't been able to determine what the current recommendation is for disabling tests on a platform-specific basis.

@kornholi
Copy link
Contributor

kornholi commented Mar 2, 2020

Awesome! Is there anything left to do here?

@mfarrugi
Copy link
Collaborator

mfarrugi commented Mar 2, 2020

lgtm, just need to resolve conflicts.

(sorry for losing track of this, not sure what happened)

Pulls in the version of rustfmt that is shipped with Rust 1.39.0.
Distribution path/version found in official release channel TOML. See
http://static.rust-lang.org/dist/channel-rust-1.39.0.toml, and
rust-lang/rust-forge#215 for more information on this.

Relates to bazelbuild#87.
The order of `sort`'s output depends on this variable, and different
users may have it set differently.
rustfmt is versioned independently of the other Rust tools, which
necessitates keeping track of its version separately. Binaries are also
not provided for FreeBSD, and so a separate targets list is also used.
@colatkinson
Copy link
Contributor Author

Hey all, I've fixed the conflicts and brought the branch up to date with master. Let me know if there's anything else to do!

@mfarrugi mfarrugi merged commit fe50d3b into bazelbuild:master Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants