Skip to content
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

Proposed fix for #500 and #429 #501

Closed
wants to merge 1 commit into from
Closed

Proposed fix for #500 and #429 #501

wants to merge 1 commit into from

Conversation

gwicksted
Copy link

Resolves relative path '..' and possible mixed casing of C:\ vs c:\ on Windows due to __dirname.

Resolves relative path `'..'` and possible mixed casing of `C:\` vs `c:\` on Windows due to `__dirname`.
@gwicksted
Copy link
Author

Travis CI build failed due to IE timeout (unrelated to this fix)

@gwicksted
Copy link
Author

Closing PR in hopes of triggering a rebuild with Travis CI.

@gwicksted gwicksted closed this Sep 22, 2015
@gwicksted
Copy link
Author

Reopen pull request to trigger Travis CI.

@gwicksted gwicksted reopened this Sep 22, 2015
@gwicksted
Copy link
Author

Sigh... another unrelated Travis CI error. Please accept the PR!

@csnover
Copy link
Member

csnover commented Sep 24, 2015

Thanks for the patch, do you have an upstream bug report about this problem? It seems wrong that require.resolve and __dirname would provide paths with different cases on the drive letter.

@gwicksted
Copy link
Author

I wish I did ... but I think they've rejected all previous attempts to fix related issues. Instead it remains not well documented but a "generally accepted practice" when performing string comparison on two paths especially if they might exist on a case-insensitive file system (ie. NTFS/FAT on Windows).

It has some pretty scary side effects when loading modules using "./" vs __dirname and the module cache.

One of the better tickets:

require() loads same module multiple times if case is different

Ticket that specifically outlines this issue (about half way down) when loading modules:

nodejs/node-v0.x-archive#7031

@csnover
Copy link
Member

csnover commented Sep 24, 2015

Thanks for the pointers. I see the commit that was added to Node.js that broke this between 0.10 and 0.12. (Why would you ever normalise drive letters to lowercase? When are drive letters ever represented lowercase anywhere ever? Uppercase drive letters date back to at least CP/M in the 1970s. How cou… I jus… whatever. )

I’m surprised this isn’t more of an issue more frequently.

@gwicksted
Copy link
Author

That about sums it up! I suppose nodejs is less frequently run on Windows and those of us who have been burned by this bug just instinctively normalize ... at least it hasn't reached the point of the double claw hammer. ;)

@csnover
Copy link
Member

csnover commented Sep 24, 2015

So sorry, can you send a test that causes this to actually happen reliably outside of the context of npm 3? I am trying to get __dirname to not be what I expect it to be on Windows in Node 4.1.1 and I don’t seem to be able to do so. I also don’t see the bad code from 0.11 any more in lib/path.js and can’t seem to find any equivalent code in current Node.js master or 4.x branches that would break the drive letter.

I see an installation failure when using npm 3, but only npm 3.

@csnover
Copy link
Member

csnover commented Sep 24, 2015

OK, so the issue here is:

  1. npm 3 doesn’t install dependencies into the intern/node_modules directory so everyone always has to do the symlinking
  2. By default, non-administrative users in Windows don’t have permissions to symlink.

So, you need to run as admin when installing with npm 3 on Windows. The fixdeps command can also optionally be updated to perform copies when a symlink fails with an EPERM error, if anyone cares to make that modification.

@csnover csnover closed this Sep 24, 2015
@csnover
Copy link
Member

csnover commented Sep 24, 2015

Or actually junction links seem to work fine without any permissions changes, so maybe there is a way to have fixdeps do that instead.

@gwicksted
Copy link
Author

Very good! Upon further testing you're absolutely right -- it seems they did fix that bug!

@gwicksted gwicksted deleted the patch-1 branch September 24, 2015 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants