theoremlp / rules_multitool

Bazel rules for ergonomic tool definitions
Apache License 2.0
24 stars 5 forks source link

feat: windows #45

Closed peakschris closed 5 months ago

peakschris commented 5 months ago

Support windows files and archives in lockfiles.

Limitations:

Validations:

peakschris commented 5 months ago

examples\module>bazel test //:integration_test --test_output=all

DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:122:22: downloading url: https://github.com/bazel-contrib/target-determinator/releases/download/v0.25.0/target-determinator.windows.amd64.exe to: tools/target-determinator/windows_x86_64_executable.exe
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:137:22: downloading url: https://github.com/cli/cli/releases/download/v2.44.1/gh_2.44.1_windows_amd64.zip to: tools/gh/windows_x86_64_archive
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:147:22: symlink: tools/gh/windows_x86_64_archive/gh_2.44.1_windows_amd64/bin, tools/gh/windows_x86_64_executable.exe
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override~multitool~multitool/tools/target-determinator/tool.bzl:10:10: symlink: bazel-out/x64_windows-fastbuild/bin/external/rules_multitool~override~multitool~multitool/tools/target-determinator/target-determinator.exe, external/rules_multitool~override~multitool~multitool.windows_x86_64/tools/target-determinator/windows_x86_64_executable.exe
INFO: Analyzed target //:integration_test (5 packages loaded, 29 targets configured).
FAIL: //:integration_test (see D:/udu/b/eheyrjsw/execroot/_main/bazel-out/x64_windows-fastbuild/testlogs/integration_test/test.log)
INFO: From Testing //:integration_test:
==================== Test output for //:integration_test:
/d/udu/b/eheyrjsw/execroot/_main/bazel-out/x64_windows-fastbuild/bin/integration_test.exe.runfiles/_main
D:\udu\b\eheyrjsw\execroot\_main\bazel-out\x64_windows-fastbuild\bin\integration_test: line 5: external/rules_multitool~override~multitool~multitool/tools/target-determinator/target-determinator.exe: No such file or directory
================================================================================
INFO: Found 1 test target...
Target //:integration_test up-to-date:
  bazel-bin/integration_test
  bazel-bin/integration_test.exe
INFO: Elapsed time: 2.176s, Critical Path: 0.84s
INFO: 8 processes: 6 internal, 2 local.
INFO: Build completed, 1 test FAILED, 8 total actions
//:integration_test                                                      FAILED in 0.7s
  D:/udu/b/eheyrjsw/execroot/_main/bazel-out/x64_windows-fastbuild/testlogs/integration_test/test.log

Executed 1 out of 1 test: 1 fails locally.

D:\udu\b\eheyrjsw\execroot_main\bazel-out\x64_windows-fastbuild\bin\integration_test.exe.runfiles_main is empty

Any clues? Thank you!

peakschris commented 5 months ago

(.venv) d:\workdir\rules_multitool\examples\module>bazel run @multitool//tools/target-determinator:cwd

Starting local Bazel server and connecting to it...
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:123:22: downloading url: https://github.com/bazel-contrib/target-determinator/releases/download/v0.25.0/target-determinator.windows.amd64.exe to: tools/target-determinator/windows_x86_64_executable.exe
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:123:22: downloading url: https://github.com/bazel-contrib/target-determinator/releases/download/v0.25.0/target-determinator.windows.amd64.exe to: tools/target-determinator/windows_x86_64_executable.exe
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:139:22: downloading url: https://github.com/cli/cli/releases/download/v2.44.1/gh_2.44.1_windows_amd64.zip to: tools/gh/windows_x86_64_archive
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:150:22: symlink: tools/gh/windows_x86_64_archive/gh_2.44.1_windows_amd64/bin, tools/gh/windows_x86_64_executable.exe
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:123:22: downloading url: https://github.com/bazel-contrib/target-determinator/releases/download/v0.25.0/target-determinator.windows.amd64.exe to: tools/target-determinator/windows_x86_64_executable.exe
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:139:22: downloading url: https://github.com/cli/cli/releases/download/v2.44.1/gh_2.44.1_windows_amd64.zip to: tools/gh/windows_x86_64_archive
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:150:22: symlink: tools/gh/windows_x86_64_archive/gh_2.44.1_windows_amd64/bin, tools/gh/windows_x86_64_executable.exe
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:123:22: downloading url: https://github.com/bazel-contrib/target-determinator/releases/download/v0.25.0/target-determinator.windows.amd64.exe to: tools/target-determinator/windows_x86_64_executable.exe
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:139:22: downloading url: https://github.com/cli/cli/releases/download/v2.44.1/gh_2.44.1_windows_amd64.zip to: tools/gh/windows_x86_64_archive
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override/multitool/private/multitool.bzl:150:22: symlink: tools/gh/windows_x86_64_archive/gh_2.44.1_windows_amd64/bin, tools/gh/windows_x86_64_executable.exe
DEBUG: D:/udu/b/eheyrjsw/external/rules_multitool~override~multitool~multitool/tools/target-determinator/tool.bzl:11:10: symlink: bazel-out/x64_windows-opt-exec-ST-13d3ddad9198/bin/external/rules_multitool~override~multitool~multitool/tools/target-determinator/target-determinator.exe, external/rules_multitool~override~multitool~multitool.windows_x86_64/tools/target-determinator/windows_x86_64_executable.exe
INFO: Analyzed target @@rules_multitool~override~multitool~multitool//tools/target-determinator:cwd (68 packages loaded, 322 targets configured).
INFO: Found 1 target...
Target @@rules_multitool~override~multitool~multitool//tools/target-determinator:cwd up-to-date:
  bazel-bin/external/rules_multitool~override~multitool~multitool/tools/target-determinator/cwd
INFO: Elapsed time: 10.076s, Critical Path: 0.06s
INFO: 6 processes: 6 internal.
INFO: Build completed successfully, 6 total actions
INFO: Running command line: bazel-bin/external/rules_multitool~override~multitool~multitool/tools/target-determinator/cwd
FATAL: ExecuteProgram(D:\udu\b\eheyrjsw\execroot\_main\bazel-out\x64_windows-fastbuild\bin\external\rules_multitool~override~multitool~multitool\tools\target-determinator\cwd) failed: ERROR: src/main/native/windows/process.cc(202): CreateProcessW("D:\udu\b\eheyrjsw\execroot\_main\bazel-out\x64_windows-fastbuild\bin\external\rules_multitool~override~multitool~multitool\tools\target-determinator\cwd"): %1 is not a valid Win32 application.
 (error: 193)

I think we need to pass ext into cwd rule somehow, can this have access to toolchain?

mark-thm commented 5 months ago

I think we need to pass ext into cwd rule somehow, can this have access to toolchain?

I’m guessing, but I suspect you need to set some kind of extension for the output filename of the cwd target, right now we declare the output file to be the same name as the target, but you may need to set it to name + “.sh” to have Bazel/Windows decide it’s a bash script — I’ve never run Bazel on Windows though.

peakschris commented 5 months ago

cwd and tests are passing now, thanks for the inputs that helped.

peakschris commented 5 months ago

Do we have to do all the stuff with file extensions on Windows? It seems like, for instance, the batch script for cwd didn't need the extension in order to be runnable.

Yes, windows requires files to have 'exe' 'bat' or 'cmd' extensions to be executed. There is no 'execute' file bit. The cwd batch script does end up with an extension (cwd.bat)

peakschris commented 5 months ago

something was fixed in bazel 7.1.0 (broken in 7.0.2) related to symlinks on windows. It's corrected some of the issues I'm seeing.

peakschris commented 5 months ago

I could bump .bazelversion to 7.1.0, or 7.2.0 which I'm using quite happily in my repo; any preferences?

mark-thm commented 5 months ago

I'm not sure if there's a way to guard windows support behind a Bazel version, but that'd probably be the ideal setup vs. assuming an incrementally higher minimum version. Otherwise bumping the repo to 7.1.0 is probably fine.

peakschris commented 5 months ago

I've implemented a version check that kicks in on all platforms if any windows artifacts are seen in the lockfile. And I've also bumped repo version to 7.1.0 so that check does not fire. The version check is not bulletproof; when bazel runs off a commit hash or latest it doesn't report a version so I can't check.

peakschris commented 5 months ago

Could you try a CI run?

mark-thm commented 5 months ago

Could you try a CI run?

Looks like just a buildifier failure.

peakschris commented 5 months ago

Is there some way I can see the test log from buildifier? It's passing locally

peakschris commented 5 months ago

never mind, it's failing locally on linux, passing on windows ;-). I'll fix the linux failures

mark-thm commented 5 months ago

46 should make the errors visible in CI if you rebase/merge main

peakschris commented 5 months ago

never mind, it's failing locally on linux, passing on windows ;-). I'll fix the linux failures

I think that buildifier_prebuilt has a windows bug that causes it to silently not run. I'll chase that up when I integrate it into our repo

Interestingly, they do have a way to handle invoking tools through a wrapper when runfiles are disabled. Some bash code that parses the manifest. That might be worth looking at in future.

peakschris commented 5 months ago

Buildifier is fixed, can you try another CI?

mark-thm commented 5 months ago

Interestingly it looks like buildifier.test is failing when common --nolegacy_external_runfiles is present.

mark-thm commented 5 months ago

Looks like that's a version-sensitive bug, we can bump buildifier_prebuilt to 6.4.0 and call it a day.

mark-thm commented 5 months ago

47

peakschris commented 5 months ago

awesome! thanks for your support 🥳

mark-thm commented 5 months ago

Thanks for taking the time to improve this ruleset!

peakschris commented 5 months ago

I had a bit of trouble with 7.1.x, specifically in cases where repo rules had errors bazel would hang. I'm seeing it fixed in 7.2.0, so if you get trouble with the current version that's the easy fix.

alexeagle commented 4 months ago

Neat! I'm adding multitool fetch ruff in https://github.com/aspect-build/rules_lint/actions/runs/9811753252/job/27094364261?pr=309 where I included Windows. However that repo has a test on Bazel 6. Something seems wrong because I get the new error message even for a repo gofumpt where no windows binaries were attempted.

I guess rules_lint can't offer anyone windows binaries until all users are forced to upgrade Bazel? That's probably not desirable. I'll back out the windows binary to land that PR...

mark-thm commented 4 months ago

It’s possible there’s a version of Bazel 6 with the same patch in 7.1+, happy to expand the range if you want to test that?

mark-thm commented 4 months ago

Also will note that you need Bazel 7.2.0 to fetch ruff using http_archive due to a Bazel bug present before that.

peakschris commented 4 months ago

Alex - At present the version check produces a failure on all platforms if a windows binary is referenced in the manifest and the version is less than 7.1. Instead, we could change it so that the version check only fails on windows platform, and lower versions are tolerated on other platforms. I can see plusses and minuses of that approach... what do you think?

alexeagle commented 4 months ago

thanks @mark-thm - I already have a re-implementation of download_and_extract due to that bug https://github.com/aspect-build/rules_lint/blob/562b84bdb00f076658fd94483a765303f671a07e/lint/ruff.bzl#L208-L220

@peakschris yeah I think we need to relax the fail semantics in some way, otherwise any ruleset that depends on multitool will have the same problem - you can either allow users on Bazel 6, or you can offer windows downloads, but not both. Your proposal to fail only when running on a windows host SGTM.

peakschris commented 4 months ago

@alexeagle there is a fix in https://github.com/theoremlp/rules_multitool/pull/50