search

vadimdemedes / ronin

Toolkit for killer CLI applications
http://vdemedes.github.io/ronin
MIT License
299 stars 15 forks source link

Updating to 0.3.5 breaks existing apps on Windows #13

Closed pulkitsinghal closed 9 years ago

pulkitsinghal commented 9 years ago

Updated a ronin based project from "ronin": "0.1.2" to "ronin": "0.3.5" and ran npm install

But afterwards I realized that something is wrong with the command path resolver because I keep running into errors like:

> vend-tools export-all-products
`Cannot find module 'C:\dev\vend-tools\commands\c:\dev\vend-tools\commands\export-all-products.js'
> vend-tools COMMAND
Cannot find module 'C:\dev\vend-tools\commands\C:\dev\vend-tools\commands\COMMAND.js'

This doesn't happen on mac.

vadimdemedes commented 9 years ago

Thanks for reporting the issue, will check what might be the problem here.

vadimdemedes commented 9 years ago

Can't reproduce, I would appreciate if you submitted a PR. Tip: the error's root must be in Program#setupCommands (lib/program.js, line 80).

pulkitsinghal commented 9 years ago

I'll do my best to provide a PR but what if I gave you access to a windows VM in the Azure cloud, that has this problem exactly reproduced? Would you consider having a peek there if I fail to fix the prob on my side?

vadimdemedes commented 9 years ago

I don't think it should be that hard. Just put console.log's in a few places inside setupCommands(), so that you see how the error comes up. It's because of a wrong path, that's being constructed in setupCommands() method.

pulkitsinghal commented 9 years ago

Ok figured out the problem ... although I would be very interested in trying to find out what is different in our windows machines (i'm on windows7 and node v0.10.33) ... since you couldn't reproduce.

Moving the convo to gitter.

pulkitsinghal commented 9 years ago

so line 86 is the culprit i suppose: path = path.replace(join(_this.path, "commands"), "").replace(separator, "");

@vdemedes - the substitution fails because _this.path is C:\blah\blah\ whereas the path is c:/blah/blah/ so the match does not happen because of forward/back slash mismatch?

vadimdemedes commented 9 years ago

Well, this should not happen, because I'm using Node's path.join() everywhere. Weird.

vadimdemedes commented 9 years ago

Similar issue here joyent/node#2309. But it was 4 years ago, so shouldn't be a problem now. What does __dirname in your entry script return?

vadimdemedes commented 9 years ago

Found a reason - https://www.npmjs.com/package/glob#windows

pulkitsinghal commented 9 years ago

@vdemedes instead of path.replace("/", separator) please use: path.replace(/\//g, separator) as that is what worked when I tested on windows7 ... I left a comment inline w/ your ealier commit: https://github.com/vdemedes/ronin/commit/c12378771f383bd393c037deec0404adbc5fb72d#diff-f58bf85b407228fb1ceb45c5f8748e6cR102

vadimdemedes commented 9 years ago

Such a stupid mistake. Fix coming, thank you.

pulkitsinghal commented 9 years ago

@vdemedes - its baaaaack!

It works on one windows7 machine but on another, I see this output:

C:\>vend-tools
[ 'c:/dev/.nvmw/v0.10.36/node_modules/vend-tools/commands/configure.js',
  ...
  'c:/dev/.nvmw/v0.10.36/node_modules/vend-tools/commands/report-costs-for-suppliers.js' ]

c:/dev/.nvmw/v0.10.36/node_modules/vend-tools/commands/configure.js
C:dev\.nvmw\v0.10.36\node_modules\vend-tools\commands\configure.js

...

error  Cannot find module 'c:\dev\.nvmw\v0.10.36\node_modules\vend-tools\commands\C:dev\.nvmw\v0.10.36\node_modules\vend-tools\commands\configure.js'

cc @elbrando