keith / buildifier-prebuilt

A bazel toolchain for using prebuilt binaries for buildifier and buildozer
MIT License
35 stars 13 forks source link

feat: initial support for Windows (amd64 only) #69

Closed GZGavinZhao closed 1 year ago

GZGavinZhao commented 1 year ago

Fixes #68.

Still need to fix tests under the tests directory, should be pretty quick though. Just want to submit it for early review. Commit message will also be formatted shortly. Done.

When running the integration tests on Windows, for now one must manually enter the examples/ directory and build the targets there because rules_bazel_integration_test has a circular dependency on buildifier_prebuilt. Trying to invoke integration tests directly from the root WORKSPACE will emit a Buildifer toolchain not found error. Once this version is uploaded to BCR this error should go away.

Currently exclude_patterns is not supported, since the dir.exe doesn't support this.

Additionally, integration tests on Windows will fail. The speculated reason is that Bazel-in-Bazel makes some runfiles disappear on Windows. However, running this rule against real-world project shows no issues.

keith commented 1 year ago

looking good! can you add windows CI to this too? https://github.com/keith/buildifier-prebuilt/blob/main/.github/workflows/ci.yml

GZGavinZhao commented 1 year ago

So um I think I've messed up the batch script. On my Linux I have the shell report the exit code of all commands that exited abnormally, but the Windows terminal doesn't do that by default, so I thought the batch runner script ran fine while in fact it always errors out and explodes. I'll try to fix the batch script today and report back later.

GZGavinZhao commented 1 year ago

Just want to verify the behavior of runner.bash.template:

Am I understanding this correct?

keith commented 1 year ago

looks right. to understand the intent there i think the important point is just that if you're running a tool just as an executable in the root of your workspace, all the bzl files will just be standard files (and symlinks within the repo will be duplicative). But if you're running as a test bazel will mirror the entire source tree as symlinks (at least on *nix operating systems, not sure about windows?) so -type f wouldn't discover them. as long as files are discovered in both cases I don't think you have to mirror the same behavior if it differs on windows

GZGavinZhao commented 1 year ago

@keith I believe the batch error is fixed; would you mind starting the CI?

keith commented 1 year ago

Started. Will review tomorrow

GZGavinZhao commented 1 year ago

Sorry for the noise. I believe this is all done except for the tests.

The current situation for testing on Windows is:

  1. In the CI, the labels //... somehow gets parsed to /.... Putting them in double quotes don't help. I could be dumb and completely missed something, so help is appreciated.
  2. Integration tests will fail because of runfiles issues. For some reason, maybe due to Bazel-in-Bazel weirdness, the runfiles directory during integration tests are empty while it should provide the needed runfiles, hence runner.bat cannot find buildifier.exe. However, running the rules in real-world projects succeeds.

I don't think 2 is something I can figure out in a short amount of time, so should we care about 1 given that tests will fail anyway?

GZGavinZhao commented 1 year ago

I've fixed the CI. I found that for some reason /// gets parsed to // on Windows, but changing that would affect other platforms, so I just removed the // at front, so the test commands become bazel test ... and bazel test :all_integration_tests. I also skipped integration tests on Windows since there's still issues with rules_bazel_integration_test. Currently the CI is passing on my fork.

keith commented 1 year ago

thanks!