Windows fixes for buildifier check, fix and test by peakschris · Pull Request #89 · keith/buildifier-prebuilt

peakschris · 2024-06-27T21:14:35Z

This PR includes fixes to support check, fix and test on Windows. Fixes #88

Tested with bazel 7.2.1 and both --noenable_runfiles and --enable_runfiles
All validations performed with --windows_enable_symlinks enabled

for buildifier.test to work, I used the following config. I could not get test to work without specifying workspace and no_sandbox.

buildifier_test(
    name = "buildifier.test",
    srcs = ["BUILD"],
    exclude_patterns = [
        "./.git/*",
    ],
    lint_mode = "warn",
    no_sandbox = True,
    workspace = "WORKSPACE",
)

keith · 2024-06-28T04:34:53Z

any way we can verify the previous failure on CI?

peakschris · 2024-06-30T22:06:43Z

@keith @cgrindel I've merged #90 here. Please would you run this CI?

Expected results:
Windows: all tests pass
Linux: 1 test fails. 4 of 6 sub-tests pass. 2 fail as the linux path does not do manifest lookups.

How should we handle the linux failure? change the test to ignore that failure and raise an extra defect?

Thanks, Chris

keith · 2024-06-30T22:58:53Z

Started CI

peakschris · 2024-06-30T23:08:18Z

@keith thank you. Results as expected. Would to like me to disable the two failing cases for non-windows?

keith · 2024-06-30T23:32:14Z

Looks like it failed on Linux but not windows? Maybe misunderstanding what you expected?

peakschris · 2024-06-30T23:39:09Z

I added new cases within my new test for:

buildifier invoke (norunfiles) - passes windows & linux
buildifier check (norunfiles) - passes windows, fails linux
buildifier fix (norunfiles) - passes windows, fails linux
buildifier invoke (w/runfiles) - passes windows & linux
buildifier check (w/runfiles) - passes windows & linux
buildifier fix (w/runfiles) - passes windows & linux

The linux failures are demonstrating existing problems (or limitations), and are not addressed by this PR.

If you agree, I can I open a new defect and disable these two cases on linux for now. Most people won't hit these as no runfiles on linux is uncommon. I won't have time to work on these.

Cheers, Chris

peakschris · 2024-07-01T08:21:46Z

I have opened #91 for the linux issue, updated the test cases.

@keith @cgrindel Please rerun CI, I'd expect all to pass now.

cgrindel · 2024-07-01T13:14:12Z

I just approved CI.

peakschris · 2024-07-01T14:11:29Z

@cgrindel could you rerun this one?

peakschris · 2024-07-01T14:17:59Z

Awesome! Windows & Linux are clean... Mac seems to be waiting on a runner... What are next steps with this PR?

cgrindel · 2024-07-06T13:57:03Z

tests/buildifier/BUILD

+    data = [
+        "//tests/unittest",
+        "@bazel_tools//tools/bash/runfiles",
+        "//:WORKSPACE",


Can you explain why you are exporting the WORKSPACE and including it in this test? If it is to create a workspace for the test, it is better to create a new WORKSPACE that refers to the parent using local_repository.

@cgrindel sorry for the delay, if you look at runner.bat.template line 24, we need to find the absolute path to the workspace directory (it is available through envvar during bazel run, but not bazel test), and there is no good way in bat/bazel to do this other than looking at the target of a symlink. Exporting and depending on WORKSPACE gives us this symlink. Any other file in root folder of workspace would also be fine.

Are you trying to find the root of a test workspace? I would expect a buildifier test to create a workspace, execute buildifier, and then verify the changes. I don't believe that any of that requires access to this repository's WORKSPACE file.

buildifier_test already has sandbox escape mechanism, implemented as the 'sandbox' and 'workspace' attributes to buildifier_test macro. This is documented here:

buildifier-prebuilt/buildifier/factory.bzl

Line 69 in 32e63ed

"no_sandbox": attr.bool(

, and is one of the things that this PR fixes on windows.

Perhaps, I am not familiar enough with this use case. I will defer to @keith.

rdesgroppes · 2025-09-25T17:35:49Z

I was about to work on a fix but found out you already did it. May I ask what remains to be done here?

This applies Windows fixes proposed by: keith#89

Currently, `buildifier` targets don't work out of the box on Windows - credits to Ofek for reporting. Sadly, it turns out that two separate attempts have already been made to remedy this: 1. In the upstream `bazelbuild/buildtools` (`go` source, but `workspace` suitable): bazelbuild/buildtools#1230 (merged) 2. `keith/buildifier-prebuilt` (`bzlmod` suitable, providing slightly outdated `go` binaries published by `bazelbuild/buildtools`): keith/buildifier-prebuilt#89 (well tested, but pending CI feedback... for months!) Inspired by Tony's [tips](https://datadoghq.atlassian.net/browse/ABLD-174?focusedCommentId=2644283), the present change proposes the "least intrusive" change so that: - we don't need to start a third effort (rules, `.bash` and `.bat` scripts), - we benefit from the validated and merged upstream behavior, - we don't depend on the goodwill of `keith/buildifier-prebuilt` to publish the version `8.2.1` of the binaries (nor subsequent versions), - we don't have to manage a complex patch, but instead simply inhibit the upstream lines that we do not need, given `buildifier` implementations are provided by `multitool` again. In short, this change combines the best of both worlds through a simple indirection made possible by the configurability of upstream rules which, unlike `keith/buildifier-prebuilt`, allow us to define which implementation of `buildifier` to use.

@ofek

### Motivation Currently, `buildifier` targets don't work out of the box on Windows - credits to @ofek for reporting. Sadly, it turns out that two separate attempts have already been made to remedy this: 1. In the upstream `bazelbuild/buildtools` (`go` source, but only `workspace`-suitable): bazelbuild/buildtools#1230 (merged) 2. `keith/buildifier-prebuilt` (`bzlmod`-suitable, and providing -slightly outdated- `go` binaries published by `bazelbuild/buildtools`): keith/buildifier-prebuilt#89 (well tested, but pending CI feedback... for months!) ### What does this PR do? Inspired by some of @aiuto's [tips](https://datadoghq.atlassian.net/browse/ABLD-174?focusedCommentId=2644283): > [1.b.] instead of pointing to the toolchain type defined in buildifier_prebuilt, point to buildifier via multitools. > [2.a.] revert prebuilt_buildtools.json to 5fd6373 The present change proposes the "least intrusive" change so that: - we don't need to start a third effort (own rules, `.bash` and `.bat` scripts), - we benefit from the validated and merged upstream behavior, - we don't depend on the goodwill of `keith/buildifier-prebuilt` to publish the version `8.2.1` of the binaries (nor subsequent versions), - we don't have to manage a complex patch, but instead simply inhibit the upstream lines that we do not need, given `buildifier` implementations are provided by `multitool` (again). In short, this change combines the best of both worlds through a simple indirection made possible by the configurability of upstream rules which, unlike `keith/buildifier-prebuilt`, allow us to define which implementation of `buildifier` to use. ### Describe how you validated your changes I used another branch to verify this works under Windows as well: https://github.com/DataDog/datadog-agent/blob/026042234ee811fd1641a2417893c08008860186/.gitlab/bazel/build-deps.yaml#L39

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 disinct community efforts wrt runner script templates, 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 like for 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 templates, - backward compatibility: aliases keep `//buildifier:buildifier*` working by delegating to `//buildifier/cmd:buildifier*`.

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*`.

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: - #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*`.

vadikmironov · 2026-01-21T22:22:53Z

One point that might be missing is the handling of ++ characters in bzlmod case which I think would still be failing after this patch. Though given that this one goes on for 18 months+ it would be great to merge as is and then add ++ handling as a minor patch on top of this one as a separate issue (trivial powershell or some not so trivial workarounds without powershell). @keith , @cgrindel , would it be possible to re-trigger CI and if all great to merge this one?

cgrindel · 2026-01-21T22:39:57Z

I don't see an option to trigger CI again.

keith · 2026-01-22T00:55:10Z

rebased in order to get it to run again

vadikmironov · 2026-01-24T13:43:25Z

rebased in order to get it to run again

@keith, thanks a lot - this is now all green. Anything else left to do on this PR or is it ready to be merged?

peakschris mentioned this pull request Jun 30, 2024

Test: new test cases showing existing issues on windows #90

Open

cgrindel reviewed Jul 6, 2024

View reviewed changes

limdor mentioned this pull request Jan 3, 2025

buildifier on Windows not working #99

Open

peakschris force-pushed the windows branch from 9eabec1 to 6c25f33 Compare August 25, 2025 16:44

rdesgroppes added a commit to rdesgroppes/buildifier-prebuilt that referenced this pull request Sep 26, 2025

Apply fixes from keith#89

600839b

rdesgroppes added a commit to rdesgroppes/buildifier-prebuilt that referenced this pull request Sep 26, 2025

Anticipate Windows fixes for buildifier

48d799c

This applies Windows fixes proposed by: keith#89

rdesgroppes mentioned this pull request Sep 29, 2025

[ABLD-174] Compose buildifier from upstream & multitool DataDog/datadog-agent#41336

Merged

rdesgroppes mentioned this pull request Sep 30, 2025

buildifier: decouple runner scripts from binaries bazelbuild/buildtools#1398

Merged

windows fixes

2349541

keith force-pushed the windows branch from 6c25f33 to 2349541 Compare January 22, 2026 00:54

Conversation

peakschris commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keith commented Jun 28, 2024

Uh oh!

peakschris commented Jun 30, 2024

Uh oh!

keith commented Jun 30, 2024

Uh oh!

peakschris commented Jun 30, 2024

Uh oh!

keith commented Jun 30, 2024

Uh oh!

peakschris commented Jun 30, 2024

Uh oh!

peakschris commented Jul 1, 2024

Uh oh!

cgrindel commented Jul 1, 2024

Uh oh!

peakschris commented Jul 1, 2024

Uh oh!

peakschris commented Jul 1, 2024

Uh oh!

cgrindel Jul 6, 2024

Choose a reason for hiding this comment

Uh oh!

peakschris Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgrindel Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

peakschris Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

cgrindel Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

rdesgroppes commented Sep 25, 2025

Uh oh!

vadikmironov commented Jan 21, 2026

Uh oh!

cgrindel commented Jan 21, 2026

Uh oh!

keith commented Jan 22, 2026

Uh oh!

vadikmironov commented Jan 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

peakschris commented Jun 27, 2024 •

edited

Loading

peakschris Dec 31, 2024 •

edited

Loading