Add Windows support

[benvanik]

Currently things seem pretty unhappy on Windows, and it'd be really awesome if support was added. fsnotify already supports Windows, which from a brief scan of the code looks like the most platform-specific thing.

• Windows 10
• Go 1.9.2
• Bazel 0.8.1

Attempting bazel build //ibazel from cmd leads to:

λ bazel build //ibazel
WARNING: C:/users/ben/appdata/local/temp/_bazel_ben/fa1bxbsu/external/com_github_bazelbuild_bazel_integration_testing/WORKSPACE:1: Workspace name in C:/users/ben/appdata/local/temp/_bazel_ben/fa1bxbsu/external/com_github_bazelbuild_bazel_integration_testing/WORKSPACE (@build_bazel_integration_testing) does not match the name given in the repository's definition (@com_github_bazelbuild_bazel_integration_testing); this will cause a build error in future versions
ERROR: D:/tools/bazel-watcher/ibazel/BUILD:30:1: no such package '@com_github_fsnotify_fsnotify//': failed to fetch com_github_fsnotify_fsnotify: 2017/12/19 18:02:56 --remote should be used with the --vcs flag. If this is an import path, use --importpath instead.
 and referenced by '//ibazel:go_default_library'
ERROR: Analysis of target '//ibazel:ibazel' failed; build aborted: no such package '@com_github_fsnotify_fsnotify//': failed to fetch com_github_fsnotify_fsnotify: 2017/12/19 18:02:56 --remote should be used with the --vcs flag. If this is an import path, use --importpath instead.
INFO: Elapsed time: 1.763s
FAILED: Build did NOT complete successfully (19 packages loaded)

This may be a go_repository issue.

If running from the msys bash the above doesn't trigger but instead nothing happens:

$ bazel build //ibazel
WARNING: C:/users/ben/appdata/local/temp/_bazel_ben/fa1bxbsu/external/com_github_bazelbuild_bazel_integration_testing/WORKSPACE:1: Workspace name in C:/users/ben/appdata/local/temp/_bazel_ben/fa1bxbsu/external/com_github_bazelbuild_bazel_integration_testing/WORKSPACE (@build_bazel_integration_testing) does not match the name given in the repository's definition (@com_github_bazelbuild_bazel_integration_testing); this will cause a build error in future versions
INFO: Analysed 0 targets (0 packages loaded).
INFO: Found 0 targets...
INFO: Elapsed time: 0.345s, Critical Path: 0.00s
INFO: Build completed successfully, 1 total action

Interestingly a git prompt window will pop up for a split second, which looks like it's running the workplace_status.sh script.

I'm not sure what issues may lie beyond those. I am but a simple C++ developer and the world of go packaging frightens and confuses me :)

[achew22]

Ben, thanks for taking a look!

Let's deal with the errors first since they are the most fun.

WRT your first compiler error. I think that is an issue with rules_go (probably worth reporting, they are really nice). I believe the issue is that when the helper that fetches code for dependencies during the fetch stage is unable to find the git CLI, but I also wouldn't be surprised if that weren't the case. It's definitely something to do with this tool. If you want to go spelunking you could try running the build again with the -s option which will print out the command that Bazel is about to run and the directory it will do it in before running it. You could then invoke the command yourself from the rules_go repo but with some debugging statements.

To the second one, it looks like //ibazel isn't a target in that repo. That's weird. What do you get as the output of bazel query //ibazel:*? If you could reply with that it would be appreciated.

Interestingly a git prompt window will pop up for a split second, which looks like it's running the workplace_status.sh script.

Yep, that's workplace_status.sh trying to do its magic. That's how we set the version string during linking.

Currently things seem pretty unhappy on Windows, and it'd be really awesome if support was added. fsnotify already supports Windows, which from a brief scan of the code looks like the most platform-specific thing.

Interestingly, in my simple experiments of just getting it to compile for Windows, fsnotify has not been a problem. That's not to say it won't be a problem in the future, but right now it seems okay. The primary issue is this single line of code. I treat the subprocesses as a process group (I think they are AKA ProcessTrees in Windows) and when I kill the group. In POSIX land this means I kill all children and descendants + the parent process that I directly invoked. I know next to nothing about how processes work in Windows and I don't know how to do that in Windows. I just did a little looking around and there might be a library that does this.

If you wanted to take on the task of figuring that out, it shouldn't be extremely difficult once you get ibazel compiling in Windows. So let's focus on that first. In the meantime, I will try and block out some time to put a stub in place to show how to get different code running in Linux vs Windows but I need to land some of the PR that are in limbo right now.

I'm not sure what issues may lie beyond those. I am but a simple C++ developer and the world of go packaging frightens and confuses me :)

By the time you're done here, you'll be a full Golang convert 😉!

[benvanik]

Thanks for the response!

There's some definite weirdness happening under the msys bash - I've never seen issues like it, but perhaps it's new bazel changes. I'm willing to write msys off - I think bazel's general guidance under Windows is to use native cmd, I was just grasping at straws :)

As for the -s, unfortunately no additional output comes out - my guess would be because the errors are happening prior to the execution of any actions. Running a bazel fetch //ibazel also fails but for all repos at once.

RE the .sh, I wonder what the bazel recommend thing to do here is. Bazel doesn't require bash on Windows, which means that the .sh is running at all is just some artifact of my machine configuration (probably because I had my git install add itself to path and register some extensions). In genrules/etc the bazel guidance has been to provide both .sh and .cmd files, but I don't know a good way to switch which one is run from a bazel.rc flag. When I was doing some extra_actions work I ended up using python instead of bash scripts to bypass the whole issue (and avoid writing bat files ;)

I've dug into the rules_go stuff a bit and there's definitely some weirdness here that's not ibazel specific. So besides the real issue you mentioned with the processes and the .sh we can probably ignore the busted go rules. I'll try to file some bugs there, as I should be able to repro them with some trivial repos :)

[achew22]

What happens if you run the commands again after commenting out

bazel-watcher/tools/bazel.rc

Line 1 in 1c4b514

build --workspace_status_command=tools/workplace_status.sh

[benvanik]

Still the death with the other errors, unfortunately. I did add some random prints and changes to how env_execute works inside of rules_go and made some progress, then noticed that there was a commit in rules_go relating to Windows today. I changed the ibazel-watcher workspace to point at the latest master commit 8968f4061342f30836ffc9a88d7e423039ce0435 from bazel-contrib/rules_go#1169 with those changes.

Now there's some new interesting errors, which again look like rules_go badness - I'll poke them over there and see what they say - that commit message claims the tests should pass, though, so either there's a missing test or my environment is borked :)

[elvisbegovic]

same here windows10

[djleonskennedy]

any news ?

[alexeagle]

Thanks for the reminders @djleonskennedy I'll make sure this goes on our hotlist with the Bazel team.

[djleonskennedy]

@alexeagle Thank you

[alexeagle]

/cc @dslomov

[GeertVL-zz]

If I do the same I get:

ERROR: C:/users/gvanlaethem/_bazel_gvanlaethem/dkwi26uz/external/go_stdlib_windows_amd64_cgo/BUILD.bazel:4:1: GoStdlib external/go_stdlib_windows_amd64_cgo/pkg failed (Exit 1)
runtime/cgo

exec: "C:\Program": file does not exist

[ronnyek]

So this is still outstanding, no go? I'm not a go developer so I unfortunately couldn't contribute much (much of any value anyway) but it seems like based on experience with other languages and technologies, seems like we should be able to just gather info about environment and have conditional calls for this sort of thing.

[GeertVL-zz]

I will give it a go and try to implement it. Right now we do it in conjunction with Bash for Windows. It works but it is not ideal.
@ronnyek it is indeed the same as I was thinking, using conditional calls.

[achew22]

@ronnyek @geertvl, the traditional way of making code target a platform in Go is to construct an interface that is shared between various OSes and then use build tags to include the environment specific code. If you try to use conditional calls you will discover that the golang libraries expose different interfaces on different OSes and as a result your code won't compile on any of them.

The way I would do it would be in a few phases:

1. Think about what a good interface for interacting with an inode watch is. This interface may be different than what is exposed by any OS.
2. Extract the existing code into the interface
3. Write the code for the Windows implementation and validate it using e2e tests (fortunately we have these already)

If you want help with anything in there, feel free to ping the thread with questions and I'll be as helpful as I can!

[jchv]

Whoops, I didn't notice this issue.

I made a quick attempt at porting this to Windows just to see what it would entail. I got some basic functionality working, though unit tests do not pass on Windows and I have not touched e2e tests at all. It'll probably be fairly tricky to get this all working right on Windows, but I'd be happy to try to get my pull request to a useful state.

If anyone else is still working on this, I suppose it would be wise for us to merge our efforts.

For now, I will post the PR and hopefully we can get it in the right shape to merge. Sorry if I duplicated anyone's efforts, I didn't actually expect for my effort to lead to a working binary so quickly :)

[dslomov]

@jchv awesome, thanks!!!

[alexeagle]

@jchv any update on your PR?

[jchv]

Yep, actively being worked on over in #144. A bit stalled on getting e2e tests working, but most other things have progressed and on my branch ibazel seems to be working properly on Windows in my limited usage.

[jchv]

Now that #144 is merged, we should probably add some tracking bugs for the remaining blockers. As far as I'm aware the most pressing matter is getting e2e tests running. Is there anything else?

[achew22]

@jchv, I think that is the only remaining blocker before I create Windows binaries for release. Right now you can build your own for Windows, but it isn't officially supported.

[achew22]

Remaining issues:

• Fix the e2e tests in Windows #213 to fix the e2e tests for Windows

[jchv]

Great, thanks. I'll take a look upstream some time and see what's blocking us on e2e, maybe I can at least make some progress.

[ronnyek]

Looks like #213 was closed. Should bazel-watcher work with windows for any of the recent releases? I just installed via npm and get the error:

FATAL: Your platform/architecture combination NaN is not yet supported.

[ocombe]

Same issue here, installed the latest version (marked as windows compatible) from npm and I get this error message as well

[jchv]

It may not yet be available via npm at this point, but you can download a Windows binary from the latest GitHub release: https://github.com/bazelbuild/bazel-watcher/releases/tag/v0.10.0

[achew22]

@ocombe, Did you attempt this through NPM? If so, I don't have a windows box to test my launcher script on so I was flying by the seat of my pants. launcher is really quite simple and if you wanted to take a stab at fixing it. The code is right here https://github.com/bazelbuild/bazel-watcher/blob/master/release/npm/index.js

It turns out I was subtracting string from string instead of delimiting. If you replace the index.js file with that it will give you a much more helpful error message at the least. Could you either make that change to the file locally, or replace it with the one that is on master and tell me the new error output?

[romain10009]

@jchv strangely, i cannot run the installator of release 0.10.0 on windows 10 insider.

[alexeagle]

@achew22 how is it going with getting a google windows laptop? I'll inter-office you one (never joking)

[ronnyek]

I was able to execute the ibazel executable listed as a release no problem. This isnt a windows insider build, and it did the usual "this software vendor is not recognized as trusted" or whatever that little message is, but you can choose to execute anyway... if I deleted ibazel from my npm home in app data, and replaced with this executable renamed... it at least executes without further error.

I'm diving into testing the actual functionality itself now. Thanks for the tips!

[rerion]

Since this issue is still open
https://github.com/bazelbuild/bazel-watcher/blob/master/release/npm/index.js#L32-L33
that doesn't seem correct. String for Windows should be 'win32'.

https://nodejs.org/docs/latest-v4.x/api/os.html#os_os_platform

[achew22]

@rerion I would love to get a PR to address this. I'm happy to release if you send the fix in

[achew22]

I think this issue is solved. If you run into more issues please feel free to open new issues.