Repository navigation
build: add support to build rules_zig on Windows - #687
AlexTereshenkov wants to merge 26 commits into
Conversation
|
I am not sure whether we want to support building the project on Windows as it means we'd need to ensure we don't use anything POSIX specific and use only tools that are compatible across POSIX and Windows. There shouldn't be much; to make the project Windows compatible, I only had to ensure the paths are compared properly (Windows path is @aherrmann What are your thoughts? I think majority of potential contributors would be on a Linux or macOS, so maybe we shouldn't bother? For the cross-platform builds, we definitely want to have it, however. If you agree, I could make it a separate PR. |
| targetPattern: "//..." | ||
| tagFilters: "" | ||
| # No remote cache or execution while bringing up Windows. | ||
| remoteEnabled: false |
There was a problem hiding this comment.
BuildBuddy does support Windows (claimed in https://www.buildbuddy.io/docs/rbe-platforms/) for RBE, but since we'll test running on Windows a binary that was built on Linux, I thought we might want to use the same type of runner?
There was a problem hiding this comment.
I'm not sure if the free BuildBuddy offering (https://www.buildbuddy.io/pricing/) offers RBE on Windows, I only see Linux listed on that pricing page.
There was a problem hiding this comment.
Oh right,
darwin and windows are currently only available for self-hosted executors
Please never mind.
aherrmann
left a comment
There was a problem hiding this comment.
Thank you for pushing this forward!
I have mostly only nits.
if that's of interest, we could perhaps move this part into a separate PR and have a more evolved matrix there to test all possible combinations (i.e. host / target platforms Windows, Linux, macOS).
Yes, placing that into a separate PR sounds good. I'm wondering about the best available approaches there that don't make CI too slow. Some completely unchecked ideas:
- GHA driven approach: first pass matrix generates and stores cross-platform artifacts; second pass matrix tests each on its target platform. (that's what's laid out here)
- Bazel RBE driven approach: cross-compile binaries to different target platforms on each host, make them an input to a test target that routes to the target platform via RE. (requires RE runners on the target platforms, we may not have that here)
- emulation approach: cross-compile binaries to different target platforms on each host, make them an input to a test target that uses emulation/API-compat-layers (wine/darling/...) to run the binary. (hermetically providing those emulators is probably not easy today)
| if: inputs.folder == '.' && inputs.targetPattern == '//...' | ||
| # Generated files are platform independent, so Linux and macOS already | ||
| # cover this check. | ||
| if: inputs.folder == '.' && inputs.targetPattern == '//...' && runner.os != 'Windows' |
There was a problem hiding this comment.
Given that we're going through the trouble of making this step Windows compatible just below, we may as well run it on Windows to make sure that it would also work for a contributor on Windows.
There was a problem hiding this comment.
I disabled this at a first attempt as I realized it's going to be a rabbit hole (the errors started appearing quickly and were nasty to pin down). I totally agree that it would be terrific to have everything buildable on Windows, but I can't seem to get it working on two Windows laptops. It passes on CI in this action, but the fact that it fails on ordinary laptop that people might be using is concerning.
I had all kinds of issues --
- Windows disables
--enable_runfilesby default, and multirun's Python runner needs a real.runfilesdirectory - Bazel doesn't pass BUILD_WORKSPACE_DIRECTORY (or any BUILD_* variable) to a script run under --run_under, so a wrapper has to set them (I had a Python wrapper)
- bazel run starts the script in the runfiles directory, not the workspace
- wrong bash picked up (WSL): bash on a Windows PATH is often WSL's launcher, which can't see C:/ paths, so the script requires BAZEL_SH and uses that bash explicitly.
- Shebangs and nested bash calls hit WSL too: #!/usr/bin/env bash resolves bash through PATH, so the wrapper puts the chosen bash's directory (in /c/... form) first on PATH and exports BAZEL_SH for multirun's inner calls
- Bazel server restarts (open): The server started from your terminal used system32\bash.exe, while nested bazel query calls use c:/msys64/..., so each nested call restarts the server.
- xargs: environment is too large for exec (open): MSYS xargs fails in update_deleted_packages even after trimming the environment to about 7 KB
After hours of troubleshooting I got some steps in utils:update succeed, but it bloated the logic so much that I start wondering whether we shall proceed at all. The environment might be so different between Windows setups, so I fear even if we make it all work on my two laptops, some other nasty problem might manifest itself on another user's machine.
Options:
- Run build and test on Windows runner to ensure compatibility (some additional platform is always great to build on, I reckon?), but don't support running
//util:updateon Windows. If a contributor wants to update the BUILD files, they would need to use WSL (or make changes manually). - Abandon the idea of making builds Windows friendly and focus on the cross-platform builds in a separate PR.
Do you see any other path forward? @aherrmann
There was a problem hiding this comment.
Apologies, I misread this as the Windows file generation checks already working in principle, but just being excluded as redundant. Of course, if this is a litany of random Windows compatibility fixes to get these checks working, then those do not all need to be covered at once. I'm all in favor of small incremental steps. So, let's focus this PR on the stated goal of getting the build and test of //... working. I think long term it would be good to get the file generation steps working, if only since each of those also indicates a likely hermeticity issue. But, that can be done gradually, and may also require some work on third-party dependencies, e.g. regarding that multirun issue.
| targetPattern: "//..." | ||
| tagFilters: "" | ||
| # No remote cache or execution while bringing up Windows. | ||
| remoteEnabled: false |
There was a problem hiding this comment.
I'm not sure if the free BuildBuddy offering (https://www.buildbuddy.io/pricing/) offers RBE on Windows, I only see Linux listed on that pricing page.
| # Minimal smoke test to verify that rules_zig builds and tests on Windows. | ||
| # Not yet part of all_tests, so a failure does not block merging. | ||
| test-windows: |
There was a problem hiding this comment.
Makes sense to start here, the long term goal is to add windows to the os matrix, correct?
There was a problem hiding this comment.
Correct. I've tried to enable it under matrix but that led to explosion of errors under the e2e (bazelrc configuration needs to be adjusted and some extra flags need to be passed + some way Zig works with dynamic libraries need (dll) need to be take into account). At some point, however, it would be great to have it in the matrix, yes.
Co-authored-by: Andreas Herrmann <andreash87@gmx.ch>
51b3193 to
a4b2d56
Compare
2f42013 to
537af42
Compare
|
Okay, I believe this is ready for final review with all the comments addressed. @aherrmann :) |
|
Please don't forget to squash and merge when merging this PR :) |
This PR makes change in the project to be able to:
Build the project itself on a Windows machine (i.e., user clones the repo and
bazel build //... && bazel test //...just works. I was able to verify this on a personal Windows device (without WSL being involved in any way).Build a binary executable on a Linux machine and run the
.exefile on a Windows machine (testing cross-platform builds); if that's of interest, we could perhaps move this part into a separate PR and have a more evolved matrix there to test all possible combinations (i.e. host / target platforms Windows, Linux, macOS).