Add support for Windows

[jawshooah]

Following up on #161. As the title indicates, this is very much still a work in progress. Figured I'd go ahead and open this to get some feedback while I continue to work on it.

Since Travis does not yet support Windows as a platform, I'm using Appveyor for CI, and a Windows 7 VM for local testing.

[jawshooah]

@lencioni Thanks for the comments, keep 'em coming! Keep in mind that this is a wholly unadulterated commit stream, which means I haven't done any rebase/amend cleanup to eliminate obsolete commits, squash related ones together, or elaborate in commit messages.

[lencioni]

This is looking really good so far! If you think this might take a while, a lot of these could be merged as you go, which would help prevent bitrot and make future reviews a bit easier. Feel free to submit incremental pull requests of chunks that you feel good about and we'll try to get them merged in quickly.

Thanks so much for working on adding Windows support to overcommit. There are a lot of people who will benefit from your contributions here.

[jawshooah]

@lencioni @sds All changes not specifically related to Windows support have been extracted and merged in separate PRs. Thanks!

There are still some issues standing in the way:

• The scripts in libexec are run with sh, which Windows doesn't like. This causes the GerritChangeId spec to fail.
• Even with grep installed via choco install unxutils, the tests for the HardTabs, TrailingWhitespace, LocalPathsInGemfile, and MergeConflicts pre-commit hooks aren't passing. Not sure what's going on here.
• Running overcommit directly in the integration specs isn't working because the shell somehow fails to locate the ruby.exe executable. Again, not sure what's going on here.

I know there's not much you can do to help here since you don't have a Windows dev machine to play with, just thought I'd give an update. Here's the latest failed build for reference.

[jawshooah]

Apparently the grep installed with unxutils is incredibly old (2.4.2, published in 2000) and doesn't properly handle the \s character class. For example:

C:\> echo foo bar>foobar.txt

C:\> grep -IHn "\s" foobar.txt

C:\> grep -IHn " " foobar.txt
foobar.txt:1:foo bar

This causes the TrailingWhitespace, MergeConflicts, and LocalPathsInGemfile specs to fail because they use \s. Not sure what we can do about this, apart from scrapping \s in favor of [ \t].

[jawshooah]

@sds @lencioni Sorry for the lack of updates on this, I've been busy with other projects and honestly, I'm not sure how to move forward. Integration tests are failing with output like the following, and I have no idea why:

commiting 
  when a hook fails 
'"C:\Ruby193\lib\ruby\gems\1.9.1\bin\ruby.exe"' is not recognized as an internal or external command,
operable program or batch file.
    exits with a non-zero status (FAILED - 1) 
  when no hooks fail
'"C:\Ruby193\lib\ruby\gems\1.9.1\bin\ruby.exe"' is not recognized as an internal or external command,
operable program or batch file.
    exits successfully 

The GerritChangeId hook simply won't work in a cmd.exe environment without directly invoking sh or bash, which I suppose we could do by changing command in the spec when running on Windows.

But I'm at a total loss regarding the failures with C:\Ruby193\lib\ruby\gems\1.9.1\bin\ruby.exe. The correct path is C:\Ruby193\bin\ruby.exe, which appears to be in the system PATH:

where ruby
C:\Ruby193\bin\ruby.exe

Really not sure what's going on with that. It's even stranger since all the integration tests pass on my local Windows VM. Might be worth opening an issue over at appveyor/ci.

[sds]

Thanks for the heads up @jawshooah,

It's interesting that the error message you display has both single and double quotes '"..."' around the executable path. I wonder if that has something to do with the problem?

[sds]

Added some debug statements to get deeper into this, and it looks like the auto-generated overcommit.bat file that RubyGems creates when installing on Windows has a path pointing to a non-existent Ruby:

@ECHO OFF
IF NOT "%~f0" == "~f0" GOTO :WinNT
@"C:\Ruby193\lib\ruby\gems\1.9.1\bin\ruby.exe" "C:/Ruby193/lib/ruby/gems/1.9.1/bin/overcommit" %1 %2 %3 %4 %5 %6 %7 %8 %9
GOTO :EOF
:WinNT
@"%~dp0ruby.exe" "%~dpn0" %*

This leads us to the RubyGems code that generates that file which looks like it was updated in rubygems/rubygems@dcf1d124 to fix a bug that looks suspiciously like our problem.

I see in our appveyor.yml that we've explicitly pinned RubyGems to 2.3.0. That's unfortunate, as it looks like the fix is only available in 2.4.2 or later. What kind of problems were you seeing in later versions?

[jawshooah]

There are issues with building native extensions on Windows in newer versions of gem. See rubygems/rubygems#977 for a lengthy discussion on the topic. Doesn't look like it's been resolved yet 😞

[sds]

Alright, since Ruby 1.9.3 has been EOLed as of February, I have no issue with dropping Ruby 1.9.3 from the Windows test suite.

Removing it reveals a number of failures with integration tests I've added recently. I'll try to have a look at those soon and get them fixed.

[jawshooah]

Looks like there were some more calls to File.symlink/FileUtils.ln_s added as well, which should be replaced with Overcommit::Utils::FileUtils.symlink.

[jawshooah]

@sds Did some cleanup myself, whittled the errors down to the hook signing integration spec and the execute_in_background spec.

[jawshooah]

Only thing failing now is hook_signing_spec. This test run includes debugging output so we can get a better sense of what's going wrong.

[jawshooah]

Finally, all tests are passing! 🍻

We're not out of the woods yet, though. Take a look at this test run. You'll notice at the bottom that the RuboCop hook failed due to the command being too long. Not only that, but the failure went by unnoticed because we're not checking result.stderr in RuboCop#run.

This exposes a couple of problems:

• We're not splitting arguments everywhere that we should.
• Many hooks (RuboCop, in this case) don't check for error conditions outside of those that are expected when the command executes as it should.

Another problem, unrelated to this particular test run, is that many hooks grab the file name with a regex group that looks something like ^(?<file>[^:]):. This won't work on Windows, since every path begins with the drive identifier followed by a : (e.g. C:/path/to/file). We should replace groups defined as above with ^(?<file>.+):.

[sds]

Thanks for your hard work on this, @jawshooah!

I've squashed some of your commits in an effort to quiet a bit of the intermediate work here. Merged in ee50226...627a8c0.

[sds]

Is there anything else holding us back from saying Overcommit supports Windows, @jawshooah?

[jawshooah]

Given Windows' stricter limitation on command length, we should probably convert all hooks that call external commands to use "splittable" args, and do so for all such hooks in the future.

[jawshooah]

Also important to note that the GIT_TEMPLATE_DIR method of installation will not work on Windows, since Unix symlinks are not supported.