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

The path string returned when nan is imported in node 6 is wrong in some cases #601

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amatas
Copy link

@amatas amatas commented Aug 25, 2016

Addressing this: #600

With the old method to get the current directory of 'nan' module, the path isn't got properly when the parent path is in a shared resource:

vboxsrv\vagrant\NODE_MODULES\nan (which don't exists)

with the new method the current directory is obtained correctly:
\\vboxsrv\vagrant\NODE_MODULES\nan

This change makes that packages like bufferutil or ref are built correctly as they are able to get the nan headers if the working directory is inside a network resource.

@bnoordhuis
Copy link
Member

bnoordhuis commented Sep 6, 2016

LGTM but it wouldn't hurt to get one or two more people to sign off on this.

Can you update the commit log to explain what you changed and why? See git log for examples of the expected format.

@TooTallNate
Copy link
Collaborator

Sounds like a bug in path.relative() to me. If it's given an absolute network file path then it should return it untouched, I would think.

@bnoordhuis
Copy link
Member

I speculate (but am not 100% sure) the realpath changes in v6 are responsible.

path.relative('.', __dirname) calls path.resolve() on both arguments. After the switch to uv_fs_realpath(), __dirname sometimes is not what it used to be on network drives and substituted drives.

I wonder though if simply console.log(__dirname) wouldn't suffice? It's guaranteed to be an absolute path.

The latest implementation of the path.relative function of node v6 seems
to be broken when either of the "from" or "to" paths are in a network
resource of windows, removing the leading backslashes "\\" and returning
a wrong path [0].

The new approach employs the path.resolve function that returns the
absolute path to the nan headers and seems to work fine in the case of
node v6 running in a windows box.

Tests:

* Windows 10 with node 4.3.0:
  c:\>node -e "console.log(require('path').relative('.','\\\\VBOXSRV\\vagrant'))"
  returns: \\VBOXSRV\vagrant\

* Windows 10 with node 6.5.0:
  c:\>node -e "console.log(require('path').relative('.','\\\\VBOXSRV\\vagrant'))"
  returns: VBOXSRV\vagrant\

[0] nodejs#600
@amatas
Copy link
Author

amatas commented Sep 7, 2016

Can you update the commit log to explain what you changed and why?

Done!

I speculate (but am not 100% sure) the realpath changes in v6 are responsible.

I believe so, I've tested the method with the following results:

* Windows 10 with node 4.3.0:
  c:\>node -e "console.log(require('path').relative('.','\\\\VBOXSRV\\vagrant'))"
  returns: \\VBOXSRV\vagrant\

* Windows 10 with node 6.5.0:
  c:\>node -e "console.log(require('path').relative('.','\\\\VBOXSRV\\vagrant'))"
  returns: VBOXSRV\vagrant\

@amatas amatas closed this Sep 7, 2016
@amatas amatas reopened this Sep 7, 2016
@amatas
Copy link
Author

amatas commented Sep 7, 2016

Sorry, I pressed the wrong button

@amatas
Copy link
Author

amatas commented Sep 19, 2016

I've done some tests with the console.log(__dirname) and seems to work fine as well, with the advantage that it doesn't depend on the methods of the Path module. Do you all think that this could be the best solution?

@bnoordhuis
Copy link
Member

@rvagg I think you are the original author? Maybe you can chime in?

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.

None yet

3 participants