Support Windows #8

Closed
jzaefferer opened this Issue Jan 22, 2014 · 25 comments

5 participants

@jzaefferer
Owner

@fnagel has tested on Windows 7, the module isn't working as expected. It looks like creation of the symlink fails, but it doesn't log any errors.

Node.js documents support for symlinks on Windows. Should start by verifying that. Might be an issue with the relative path.

@dmethvin

Oh, I installed it yesterday and didn't notice it failed to warn about an unformatted temporary commit for me as well. Windows 8, i'll try to find some time to look at it.

@jzaefferer
Owner

I looked into this a bit. I've found that the installation fails, since it incorrectly thinks its installing "in itself", see the avoidSelfInstall function here: https://github.com/jzaefferer/commitplease/blob/1208bb2f66a5f6193ca1fd00a889b023dd9757c0/setup.js#L1-8

process.cwd() returns a path like "C:\users\..." on Windows, so that check fails, since the regex assumes a forward slash as the path separator.

I don't have a proper dev enviroment set up on Windows, so I would appreciate some help in getting this fixed.

Creating the symlink itself seems to work fine, though I don't know if the relative path will work (no git installed).

@dmethvin

I got the regex check fixed locally by using [/\\\\] to allow backslash. It looks like symlink needs an extra arg "file" on Windows to help it figure out the type, see http://nodejs.org/api/fs.html#fs_fs_symlink_srcpath_dstpath_type_callback

I can do some testing later today if needed.

@jzaefferer
Owner

I would appreciate if you could test with that updated regex. You can test with npm install path/to/local/module, then do a commit there.

The type argument is supposed to default to file, so that should be correct already.

@jzaefferer
Owner

@JamesMGreene could you give this a try?

@jzaefferer jzaefferer closed this in f234c91 Feb 5, 2014
@jzaefferer
Owner

Fixed, tested on Windows 7 with git-bash (via msysgit). The "self install" check was the only thing that was failing, creating the symlink works just fine.

@dmethvin @fnagel would be great if you could confirm that it works for you, too. 1.5.1 includes this fix.

@dmethvin

Still having problems, tried it on two repos

$ npm install commitplease
npm http GET https://registry.npmjs.org/commitplease
npm http 304 https://registry.npmjs.org/commitplease
npm http GET https://registry.npmjs.org/mout/0.8.0
npm http GET https://registry.npmjs.org/colors/0.6.2
npm http GET https://registry.npmjs.org/semver/2.2.1
npm http 304 https://registry.npmjs.org/colors/0.6.2
npm http 304 https://registry.npmjs.org/mout/0.8.0
npm http 304 https://registry.npmjs.org/semver/2.2.1

> commitplease@1.5.1 install c:\wamp\www\jquery-1x\node_modules\commitplease
> node install

fs.js:730
  return binding.symlink(preprocessSymlinkDestination(destination, type),
                 ^
Error: EPERM, operation not permitted '..\..\node_modules\commitplease\commit-msg-hook.js'
    at Object.fs.symlinkSync (fs.js:730:18)
    at Object.context.createLink (c:\wamp\www\jquery-1x\node_modules\commitplease\setup.js:32:6)
    at Object.<anonymous> (c:\wamp\www\jquery-1x\node_modules\commitplease\install.js:15:7)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:902:3
@jzaefferer
Owner

Can you provide some information about your environment? Afaik symlinks don't work on "older" Windows versions, but I don't have details on that.

@dmethvin

Node 0.10.24 and Windows 8.1 running a bash shell with msysgit.

I checked permissions and my account has full permissions on this subdirectory. Similar thread:

ronaldlokers/grunt-casperjs#46

@jzaefferer
Owner

I found nodejs/node-v0.x-archive#6342

EPERM means 'permission denied.' The default security policy allows only administrators to create symbolic links.
By the way, if you're going to deploy on Windows - you can run your application as a regular user, you just need to tweak the security policy a little.

If that's correct, there's not much I can do about it. Could you verify?

@mgol

Well, we can't add commitplease to devDependencies until it doesn't error. If the symlink issue is not fixable, we may want to add a try-catch or sth like that so that npm install doesn't fail.

@jzaefferer
Owner

Need to try if copying works without admin privileges.

@jzaefferer jzaefferer reopened this Feb 5, 2014
@fnagel

I just installed it again on Win7 when using default cmd with admin rights.
Now it works as expected!

Is there a way to pass the error messages to an git GUI? For example TortoiseGit or SourceTree?

@dmethvin

When i get a chance (later today?) I will run with Procmon (similar to strace on Linux) and see exactly what is giving the error.

@fnagel

Just tried again: installation works only with admin rights, usage without.

@jzaefferer
Owner

I can catch that error, display a more helpful error message, without having the npm install failing (exit(0) et al).

Regarding commits from a GUI: No idea yet. Would have to try that myself, ideas are welcome. Separate issue though.

@dmethvin
@jzaefferer
Owner

@fnagel did you try with multi-line commits? I actually haven't tested that myself.

@fnagel

@jzaefferer Can you give me a example and what would be expected?

Using something like

Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut 
labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et 
ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum 
dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore 
magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet 
clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.

works as expected (no successful commit).

ps: See #11 for GUI support

@jzaefferer
Owner

For example:

Setup: Normalize process.cwd() to support Windows

Fixes gh-8

Aka "Setup: Normalize process.cwd() to support Windows\n\nFixes gh-8"

@fnagel

I was able to commit your example message.

@jzaefferer
Owner

46615fb adds a more helpful error message when running the installation on Windows without admin rights. Even when installation of commitplease fails, other modules installed as part of a npm install call will still be installed. Seems good enough to me to call this "Windows support".

@jzaefferer jzaefferer closed this Feb 11, 2014
@JamesMGreene

↑ True story, as tested by me. 😛

If you want to get more extreme, you can add a dependency on node-windows and use the isAdminUser method. Probably way more effort than necessary, though.

@jzaefferer
Owner

Reopening based on discussion in jquery/jquery#1523 (comment)

Will try to just copy the file, then check the content when uninstalling.

@jzaefferer jzaefferer reopened this Apr 8, 2014
@dmethvin

The sooner the better on this, thanks!

@jzaefferer jzaefferer added a commit that referenced this issue May 15, 2014
@jzaefferer All: Replace symlink with copying file for Windows compat
Also replace colors module with chalk

Fixes #8
729b94f
@jzaefferer jzaefferer closed this in #18 May 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment