Made generated files more Buildifier compliant #260
Conversation
|
Maybe a smoother/larger change would be to add a new field that controls the formatting of the "Remote" output files and rename @acmcarther would probably have strong feelings about this. Hoping to hear from him. |
|
The |
UebelAndre
changed the title
|
@PiotrSikora see 210083e Man, that was a tough one. Friendly reminder to always challenge one's assumptions 😄 edit: This change also has your changes from #261 btw |
|
@acmcarther This PR is ready for review |
|
@PiotrSikora I want to explicitly thank you for opening your PR for sorting out the other attributes. That was SUUUUPER helpful in getting us on the same page 🙏 😄 |
|
@UebelAndre awesome! Thanks for fixing this! |
|
Note: This should probably ship as |
|
@acmcarther this is ready for another review |
|
There was still one case that was not being handled. I updated the PR to hopefully pass now but it required adding more @acmcarther Up to you if you still want to merge this. I think this is already a big improvement but don't know if that's just because I sank a bunch of time into it 😅 |
|
I'm open to merging this and implementing correct sorting of the merged default_deps and additional_deps in a separate PR, but I don't feel strongly about it and would defer to @PiotrSikora's preference. |
|
@PiotrSikora @acmcarther How does the change look now? |
Summary:
Version 0.7.0 of `cargo-raze` was just released. Some visible changes:
- can now set `[package.metadata.raze]` instead of `[raze]` to avoid a
Cargo warning: <google/cargo-raze#274>
- build files have been renamed and restructured to be more compliant
with Buildifier: <google/cargo-raze#260>
If you have an old version installed, run `cargo install cargo-raze` to
update to the most recent version.
Changes to `third_party/rust/` generated with:
```
(rm -rf third_party/rust/ && cd tensorboard/data/server && cargo raze)
```
The changes to `Cargo.toml` and `third_party/rust/` are independent.
Test Plan:
Run `(cd tensorboard/data/server && cargo test)` and note that the tests
run without an “unused manifest key: raze” warning. Note further that
everything still builds with Bazel.
wchargin-branch: rust-regen-raze-0.7.0
wchargin-source: c73530a2f7948b4842617ff686f645e66e917d0d
Summary:
Version 0.7.0 of `cargo-raze` was just released. Some visible changes:
- can now set `[package.metadata.raze]` instead of `[raze]` to avoid a
Cargo warning: <google/cargo-raze#274>
- build files have been renamed and restructured to be more compliant
with Buildifier: <google/cargo-raze#260>
If you have an old version installed, run `cargo install cargo-raze` to
update to the most recent version.
Changes to `third_party/rust/` generated with:
```
(rm -rf third_party/rust/ && cd tensorboard/data/server && cargo raze)
```
The changes to `Cargo.toml` and `third_party/rust/` are independent.
Test Plan:
Run `(cd tensorboard/data/server && cargo test)` and note that the tests
run without an “unused manifest key: raze” warning. Note further that
everything still builds with Bazel.
wchargin-branch: rust-regen-raze-0.7.0
This addresses #192 (comment) and was already almost merged but was changed at the last minute. If Buildifier expects
BUILD.{name}.bazelthencargo-razeshould be compliant. Vendored files are already compliant by default with the defaultoutput_buildfile_suffixbeingBUILD.bazeledit: Linked the wrong comment. Fixed that.