Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tools don't run under windows #78

Closed
peakschris opened this issue Jun 17, 2024 · 5 comments
Closed

Tools don't run under windows #78

peakschris opened this issue Jun 17, 2024 · 5 comments

Comments

@peakschris
Copy link

peakschris commented Jun 17, 2024

I can see that, on Windows, executable versions of tools such as buf.exe exist. But running them in the toolchain fails. This is impacting integrations such as rules_lint which refuse to lint protobuf files on windows (aspect-build/rules_lint#294)

clone rules_buf latest
cd examples\version
bazel run //:print

INFO: Analyzed target //:print (2 packages loaded, 6 targets configured).
ERROR: D:/workdir/rules_buf/examples/version/BUILD.bazel:19:18: Middleman _middlemen/print-runfiles failed: missing input file '@@rules_buf_toolchains//:buf'
ERROR: D:/workdir/rules_buf/examples/version/BUILD.bazel:19:18: Middleman _middlemen/print-runfiles failed: 1 input file(s) do not exist
Target //:print failed to build
Use --verbose_failures to see the command lines of failed build steps.
ERROR: D:/workdir/rules_buf/examples/version/BUILD.bazel:19:18 Middleman _middlemen/print-runfiles failed: 1 input file(s) do not exist
INFO: Elapsed time: 1.249s, Critical Path: 0.01s
INFO: 4 processes: 4 internal.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

If I edit toolchain.bzl and change line 61:
from: cli = str(Label("//:"+ cmd)),
to: cli = str(Label"//:" + cmd + ext)),

Then it gets further, but fails with:

FATAL: ExecuteProgram(D:\udu\b\myor77df\execroot\_main\bazel-out\x64_windows-fastbuild\bin\print) failed: ERROR: src/main/native/windows/process.cc(202): CreateProcessW("D:\udu\b\myor77df\execroot\_main\bazel-out\x64_windows-fastbuild\bin\print"): %1 is not a valid Win32 application.
 (error: 193)
@peakschris
Copy link
Author

I don't understand runfiles rules enough to fix this, but am happy to test out potential fixes, I have a windows environment ready to go.

@jchadwick-buf
Copy link
Member

You're right, there are some issues running this under Windows right now. There's a bug that registers the toolchains incorrectly, which is what you're hitting here.

I took a look and noticed also, though, that there's another issue: Bazel under Windows doesn't populate the runfiles symlinks because historically Windows didn't support symlinks, and even now that it does, symlinks are slow and make builds take a long time. Because of this, our solution for shimming protoc can't work without enabling runfiles via adding the build option --enable_runfiles (and ideally the startup option --windows_enable_symlinks.) In practice this looks like:

bazel --windows_enable_symlinks test //... --enable_runfiles

The relevant options can be added to bazelrc.

A better solution certainly needs to be worked out in the long term, but I believe this essentially eliminates the possibility of using scripting to shim protoc, so we'll need a new approach.

I'll be pushing up a few fixes shortly. Thanks for the report.

@peakschris
Copy link
Author

Thank you! We already have both --windows_enable_symlinks and --enable_runfiles for other reasons; there are a lot of other modules that also require them.

I believe it is possible to work without --enable_runfiles, although I don't know the detail of your repositories, by parsing the manifest, and this can be done natively on windows in a bat file (e.g. https://github.com/aspect-build/bazel-lib/blob/main/lib/windows_utils.bzl) or in a native exe. Anyhow, this is low priority for us as we are unlikely to be able to run with --noenable_runfiles anytime soon.

Thank you so much for the quick fix! I'll integrate it into rules_lint and see how it works.

@jchadwick-buf
Copy link
Member

I believe it is possible to work without --enable_runfiles, although I don't know the detail of your repositories, by parsing the manifest, and this can be done natively on windows in a bat file (e.g. https://github.com/aspect-build/bazel-lib/blob/main/lib/windows_utils.bzl) or in a native exe. Anyhow, this is low priority for us as we are unlikely to be able to run with --noenable_runfiles anytime soon.

Wow, I did not expect anybody to have already implemented runfiles manifest parsing in the Batch command interpreter. My very first thought was that this wouldn't be worth the effort, but all in all, the resulting code actually looks somewhat succinct and straightforward. Maybe that's the way to go after all.

@peakschris
Copy link
Author

peakschris commented Jun 17, 2024

Wow, I did not expect anybody to have already implemented runfiles manifest parsing in the Batch command interpreter

Yeah, I am impressed too!

The fix on your PR works great, BTW, we have buf linting now working on Windows in rules_lint :-) Feel free to close this or leave it open as you wish. I'm happy 👍

jchadwick-buf added a commit that referenced this issue Jun 20, 2024
This should resolve the toolchain registration issue from #78.

Also, it adds a batch script version of the shell script shim used to
invoke protoc for tests; however, this requires `--enable_runfiles` to
be explicitly passed or added to `.bazelrc` for a project, as Bazel on
Windows does not populate runfiles by default.

In the future, we will need a better approach to supporting Windows that
uses the runfiles manifest instead. This is tricky though, and will
require some careful thought.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants