buildifier: decouple runner scripts from binaries#1398
Conversation
rdesgroppes
force-pushed
the
export-buildifier-scripts-without-build-deps
branch
from
7028e17 to
f376c5b
Compare
rdesgroppes
force-pushed
the
export-buildifier-scripts-without-build-deps
branch
from
f376c5b to
8914e45
Compare
rdesgroppes
force-pushed
the
export-buildifier-scripts-without-build-deps
branch
from
8914e45 to
e72e822
Compare
rdesgroppes
force-pushed
the
export-buildifier-scripts-without-build-deps
branch
from
e72e822 to
ab54e73
Compare
AnnaSvalova
left a comment
AnnaSvalova
left a comment
There was a problem hiding this comment.
I'm not against this change, but cmd sounds a bit unusual for build tools and bazel 🤔 Maybe cli or similar?
@AnnaSvalova, I'll of course follow your call here, but I was wondering whether the Go convention should be favored in this case? The I thought it might be helpful to stick with it for familiarity. But happy to change it to
|
This change aims at making `buildtools/buildifier` more modular and easier to consume by enabling use of prebuilt binaries without pulling in unnecessary dependencies - while strengthening upstream as the authoritative home for runner script templates. Today, `buildifier/BUILD.bazel` exposes two different sets of rules: - exports for runner script templates (`runner.bash.template`, `runner.bat.template`) as well as `README.md`, - `go` build & `sh` test rules which require external repos (`rules_go`, `rules_shell`) even for consumers who don't need to build the `buildifier` binary. It is worth noticing there are two distinct community efforts on runner scripts, the present/upstream `bazelbuild/buildtools` and the derived `keith/buildifier-prebuilt` (Bazel Central Registry module). These drift apart over time, leading to unsynchronized contents (`factory.bzl`, `runner.bash.template`) and efforts, e.g. on Windows support: - bazelbuild#1230 - keith/buildifier-prebuilt#89 (open for a year) Intended benefits of this change: - decoupling: by moving `go_binary` and `sh_test` targets into `buildifier/cmd`, the top-level `buildifier/BUILD.bazel` no longer requires `rules_go` nor `rules_shell`, - flexibility: users can either build binaries from source (via `//buildifier/cmd`) or configure rules to use prebuilt binaries (e.g. via `multitool`) while still consuming the same upstream runner script templates, - backward compatibility: aliases keep `//buildifier:buildifier*` working by delegating to `//buildifier/cmd:buildifier*`.
rdesgroppes
force-pushed
the
export-buildifier-scripts-without-build-deps
branch
from
ab54e73 to
49852da
Compare
Fair point. Also, I'd say that an even more common pattern is using a project name instead of "cli" or "cmd". In any case it was a nit, LGTM. |
|
Drive-by FYI --- I think this broke the i.e. as documented here: Difference between your commit and the one prior: Edit: looks like this works now instead: |
|
Emphasizing @gunsch's comment above- we use the documented |
|
You can't win 'em all. We got broken too, so I fixed it to use the new location. Guess who just got broken again 😅 ? If anything this is a reminder using |
5 participants
This change aims at making
buildtools/buildifiermore modular and easier to consume by enabling use of prebuilt binaries without having to pull in unnecessary dependencies - while strengthening upstream as the authoritative home for runner script templates.Today,
buildifier/BUILD.bazelexposes two different sets of rules:runner.bash.template,runner.bat.template) as well asREADME.md,gobuild &shtest rules which require external repos (rules_go,rules_shell) even for consumers who don't need to build thebuildifierbinary.It is worth noticing there are two distinct community efforts on runner scripts, the present/upstream
bazelbuild/buildtoolsand the derivedkeith/buildifier-prebuilt(Bazel Central Registry module). These drift apart over time, leading to unsynchronized contents (factory.bzl, runner.bash.template) and efforts, e.g. on Windows support:Intended benefits of this change:
go_binaryandsh_testtargets intobuildifier/cmd, the top-levelbuildifier/BUILD.bazelno longer requiresrules_gonorrules_shell,//buildifier/cmd) or configure rules to use prebuilt binaries (e.g. via rules_multitool) while still consuming the same upstream runner script templates,//buildifier:buildifier*labels working by delegating to the corresponding//buildifier/cmd:buildifier*.