-
Notifications
You must be signed in to change notification settings - Fork 119
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
Add Windows support #144
Add Windows support #144
Conversation
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
The tests are failing with
On Windows we don't have runfiles tree enabled by default, it's recommend to use a runfiles library to access all the data files. |
|
@meteorcloudy Do you know what's up with the |
Looks like we're still using Bazel 0.17.2 on CI, but building runfiles tree on Windows only works with Bazel 0.18.0 or later. I'll ping you when it's upgraded. |
Hi @achew22, Bazel has been upgraded to 0.18.0 on CI, I'm rerunning the test on Windows |
Three tests are passing now
The rest are failing with:
Is it supposed to be |
Another question, does ibazel itself needs the runfiles symlink tree to run? Or it's jsut needed for the tests. |
No, ibazel takes all its data dependencies and converts them to string variables in a go_embed_data rule which means I shouldn't have any production runtime deps. I wrote this in go so it could be statically linked and distributed without any |
--script_path should write a batch file instead of bash file on Windows. Related: bazelbuild/bazel-watcher#144 (ibazel uses --script_path) RELNOTES: None PiperOrigin-RevId: 218828314
Hi @jchv , I debugged ibazel on Windows with your PR, and found From Bazel side: From ibazel side:
John, since you authored this change, can you help debug and make |
Sure, I'll take another look this weekend. I had been using There's probably bugs in |
Sorry for lack of updates. I went at it again this weekend, but I'm sad to say I don't know how to progress. So, I updated to Bazel HEAD today, and ran into some new issues, specifically in @io_bazel_rules_go//:go_context_data.
I re-enabled it. Then got this:
Sadly, looks like this one can't be re-enabled. I was hoping maybe there was a version of Bazel I could use before this deprecation, so I went ahead and built bazel at ea225fe. This didn't seem to work either, same exact issues. I continued on with this version of Bazel. The problem was fixed in io_bazel_rules_go 0.6.1. I tried that:
I traced it down to this: COPTS = select({
":msvc" : MSVC_COPTS,
"//conditions:default": [
"-DHAVE_PTHREAD",
"-Wall",
"-Wwrite-strings",
"-Woverloaded-virtual",
"-Wno-sign-compare",
"-Wno-unused-function",
# Prevents ISO C++ const string assignment warnings for pyext sources.
"-Wno-writable-strings",
],
})
OK, so maybe if I explicitly specify Well, I got past that issue. But then:
Adding This is where I am currently at. Sadly, trying to get anything working on Windows is an exercise in frustration at the moment. I suppose in order to progress further, there are more upstream changes that will need to be made, and right now I do not have the expertise to trace this any further. I might try to take another look at the problem later, but if anyone has any insight as to how I can move forward it would be appreciated. |
@jchv, Thanks so much for the work you've done here! I can't tell you how much I appreciate you putting in so much effort. I just spent the last hour doing a bit of cleanup around the repo and I think some of these problems should be fixed for you. For example, I just merged #153 which I believe fixes the --stamp problem. It turns out that Windows doesn't have a .sh interpreter so you have to use python. Who knew? I also moved the .bazelrc file into the correct location so that if you're using a modern bazel it will behave correctly (#154). I upgraded rules_go past the version you specified to 0.16.1 in (#152). I think this fixes a few more of the problems but there are almost certainly more lingering around.
@meteorcloudy is the person on the Bazel team who is responsible for Bazel on Windows and he has been really helpful to me in debugging issues. I think I fixed the stamp problem but if you see something else please comment on here and we can figure it out together! Really, thank you so much! I don't think I could have done this. |
For the record, we're back to
|
golang/go#5615 might have some more info that's useful to us here.
|
I'm wondering if this might be a problem that is finicky enough that we don't care to solve it. Poking around at the error I think it comes from the test case triggering an event in iBazel such that it needs to restart the process (and thus kills it). This wouldn't be a problem normally, but we are using process groups which means everything is hard. Maybe we can enhance our permissions when we start the process. I will now do something that I never thought I would ever do -- link to some MS win32api documentation. Shudder Looks like there is a permission that can be passed at construction time where you can allow TERMINATE_PROCESS. We may have to use the windows speific golang api to do this https://godoc.org/golang.org/x/sys/windows which exports that value. |
https://godoc.org/golang.org/x/sys/windows#OpenProcess is the API that takes that permission |
I was hoping for a windows specific SysProcAttr, but I don't think we are so lucky |
Looks like there are two problems
Control flow that is causing this problem:
|
With even more staring at the log output:
Looks like there is something hinky there. On the plus side, it is actually running in CI now. It's really late at night for me right now so I'm going to go to sleep and contemplate this in the morning. STDOUT isn't hooked up in Windows? Is that possible? |
@achew22 Thanks for making the
@jchv Yes, the protoc compile failure can be work around by With addition fixes from @achew22 , I think you'll be able to move forward now. Please ping me if you have any other question. |
I just did the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works like a charm, thank you so much for the fix!!!
@meteorcloudy, I'm not sure that's the case. It looks like the CI server isn't handling things correctly. It only ran 4 of the tests and none of the e2e tests were included. I'm aware that it is due to an incompatible change in Bazel, but it really seems like that should result in a CI failure.
|
@achew22 Yes, that is definitely a Bazel bug, filed bazelbuild/bazel#7115 To make the test work again, the fastest way is to fix bazel_integration_testing and upgrade it in bazel-watcher. As for ibazel on Windows, I manually tested |
@achew22 Can you help fixing bazel_integration_testing? I'm currently busy with other CI breakages. |
Did the e2e tests pass on your box? |
As @jchv pointed out, we need to make bazel-integration-testing work on Windows first |
The e2e test are not even running on Linux, I think that's due to the incompatible change. Let's not merge this PR until we enable them again. |
@jchv Can you rebase to HEAD so that we can trigger the presubmit again. |
@meteorcloudy I was able to trigger this from my side. Thanks for pinging the PR |
@achew22 Thanks! Looks like the Windows failures are expected, e2e tests are failing because bazel_integration_test doesn't work on Windows yet. |
I'm interested in getting the e2e tests working, but in the meantime should we mark the tests to not run on Windows? There's really no need to rush to get this PR ready to merge imo, but some Windows support is probably better than none, and it seems movements to the other projects may take a long time. (I still have a couple of other things to fix before I think it could possibly be reviewed and merged, namely the tests are still hacky.) |
@jchv TBH, I'm pretty reluctant to release into windows without any real test coverage. I know that a small amount comes in from the tests in Since I can only release to windows one time and I think there will be a flood of people (50-100 ppl), I don't want to leave a bad taste in their mouth for their first, and probably only if things go poorly, experience with ibazel. |
SGTM. Maybe to help reduce code rot I could at least split some of the less Windows-related work into other PRs to reduce the surface area, though. I am admittedly paranoid of having a large and invasive rebase down the line if it takes as long as I'm imagining to fix the upstream e2e test problems, since it's pending on something that seems to have been stalled for a while. |
@achew22 Merging this PR doesn't mean Windows support is ready and we have to do a release for Windows, right? We can still keep #105 open until e2e tests are also enabled for Windows. I think merging this change now could reduce the rebasing work @jchv had to do. Also, we'll at least have some test coverage on Bazel CI to prevent any breakage from Bazel itself. I just enabled Bazel watcher in downstream and it already caught a Bazel remote execution bug. |
You two are both totally correct and I'm 100% wrong. I'm going to tag the tests as |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) |
--script_path should write a batch file instead of bash file on Windows. Related: bazelbuild/bazel-watcher#144 (ibazel uses --script_path) RELNOTES: None PiperOrigin-RevId: 218828314
This PR makes ibazel build and run on Windows. Tests do not pass on Windows, and Process Group is not tested at all (at least not directly) so it is definitely not quite ready. I am posting it mostly to get a litmus test on the approach as well as to potentially sync efforts.
Issue: #105.