lib,test: Use long paths when loading native addons by justinmchase · Pull Request #2965 · nodejs/node · GitHub

Currently, it's possible for a native addon on windows to throw an exception during module loading if it's path is too long.

This change resolves the issue by converting the path to the addon to a long path before calling process.dlopen.

Fixes #2964

style nit: no space after function keyword

Fixed thanks.

I wanted to add that I tried making a test for this, but that would entail actually loading a native addon (I think). And I couldn't find an example of another test doing that so I'm not sure how to effectively make a test for this. I'm open for input on how best to achieve that.

but that would entail actually loading a native addon

I don't see a problem with that. It should be test/addons/ though. An example can be found here, I think: https://github.com/nodejs/node/pull/2830/files

cc @bnoordhuis?

@Fishrock123 Great, thanks. Looking now.

@Fishrock123 I apologize for the delay but based on your feedback I added a unit test that successfully loads the addon from a long path.

I tried running eslint like this:

node tools/eslint/bin/eslint.js lib/module.js --rulesdir tools/eslint-rules --quiet

But I get a ton of existing errors. The CONTRIBUTING.md documentation just says:

Make sure the linter is happy and that all tests pass.

I'm not totally sure that I'm doing it right.

For linting just run make jslint :)

On Sep 23, 2015, at 7:05 PM, Justin Chase notifications@github.com wrote:

I tried running eslint like this:

node tools/eslint/bin/eslint.js lib/module.js --rulesdir tools/eslint-rules --quiet
But I get a ton of existing errors. The CONTRIBUTING.md documentation just says:

Make sure the linter is happy and that all tests pass.

I'm not totally sure that I'm doing it right.

—
Reply to this email directly or view it on GitHub.

@Fishrock123 Again, thank you for your patience and your help :)

I ran the tests and the linter successfully. I rebased my commits into a single commit with a descriptive comment and recently rebased upstream/master into my branch. I believe that this is ready for merging.

Can you assert.equal(addon.hello(), 'world'); here?

done.

@justinmchase Are you intentionally comparing against both null and undefined here?

@thefourtheye I believe I just copied that line from another test doing something similar. However, I think its most accurate to say that addon should be neither null or undefined. So yes it is intentional.

Also, the error case before this PR actually throws an error instead of returning null or undefined. So it's mostly just a sanity check. Same with the following line which actually calls the addon.hello() function, another sanity check.

LGTM but someone from @nodejs/platform-windows should sign off on this, I'm not sure to what extent LoadLibrary() supports UNC paths.

CI: https://ci.nodejs.org/job/node-test-pull-request/401/

Also, you might want to make sure that the commit is reflected in your GitHub profile. https://help.github.com/articles/setting-your-email-in-git/

LGTM but someone from @nodejs/platform-windows should sign off on this, I'm not sure to what extent LoadLibrary() supports UNC paths.

It should work, although a comment on the doc page suggests that it doesn't for 32-bit builds. Did you test this?

When using require to load a native addon the path must be converted into a long path, otherwise the addon will fail to be loaded on windows if the path is longer than 260 characters.

@piscisaureus I didn't, I only tested on x64. I don't know if I trust that comment but I will make a VM and try it there, it could take a little while.

@thefourtheye I think the machine I happened to make this branch on hadn't been configured right but commit itself should have the right email and name now. Sorry for that.

LGTM, thanks for this it was a painful issue in the past too :D

/cc @nodejs/build - I think our Windows CI bots only test 64 bits builds now?

@bnoordhuis : correct (even though I would like to fix that).

@bnoordhuis @piscisaureus I built an x86 win7 vm and tested it there, confirmed it works on x86 as well.

I added a correcting comment to msdn to help prevent confusion in the future:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms684175(v=vs.85).aspx#12

CI: https://ci.nodejs.org/job/node-test-pull-request/414/

CI seems happy.

When using require to load a native addon the path must be converted into a long path, otherwise the addon will fail to be loaded on windows if the path is longer than 260 characters. PR-URL: #2965 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <inglor@gmail.com>

Landed in 8593b3e, thanks Justin. I had to touch up the first line of the commit log to make it fit in <= 50 columns, hope you don't mind.

When using require to load a native addon the path must be converted into a long path, otherwise the addon will fail to be loaded on windows if the path is longer than 260 characters. PR-URL: #2965 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <inglor@gmail.com>

targos reviewed Sep 19, 2015
View reviewed changes

targos added module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform. labels Sep 19, 2015

justinmchase force-pushed the use-long-path-to-load-addons branch 2 times, most recently from 066501e to 04e3ff7 Compare September 24, 2015 04:07

justinmchase changed the title Use long paths when loading native addons lib,test: Use long paths when loading native addons Sep 25, 2015

bnoordhuis reviewed Sep 30, 2015
View reviewed changes

justinmchase force-pushed the use-long-path-to-load-addons branch from 04e3ff7 to 450d406 Compare September 30, 2015 14:16

lib,test: Use long paths when loading native addons (windows only)

450d406

When using require to load a native addon the path must be converted into a long path, otherwise the addon will fail to be loaded on windows if the path is longer than 260 characters.

bnoordhuis closed this Oct 5, 2015

jasnell mentioned this pull request Oct 8, 2015

Preparing v4.x for LTS #3252

Closed

29 tasks

jasnell added the lts-landed-on-v4.x label Oct 8, 2015

jasnell mentioned this pull request Oct 8, 2015

Release proposal: v4.2.0 'Argon' (LTS) #3258

Closed

seishun mentioned this pull request Oct 15, 2015

[windows only] Require fails to load native addons with path longer than 260 characters #2964

Closed

mhdawson mentioned this pull request Nov 4, 2015

test\addons\load-long-path\test.js - windows 32 bit failures #3667

Closed

justinmchase deleted the use-long-path-to-load-addons branch February 5, 2016 17:17

lib,test: Use long paths when loading native addons by justinmchase · Pull Request #2965 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib,test: Use long paths when loading native addons - #2965

lib,test: Use long paths when loading native addons#2965justinmchase wants to merge 1 commit intonodejs:masternodejs/node:masterfrom justinmchase:use-long-path-to-load-addonsjustinmchase/node:use-long-path-to-load-addonsCopy head branch name to clipboard

Conversation

justinmchase commented Sep 19, 2015

Uh oh!

targos Sep 19, 2015

Choose a reason for hiding this comment

Uh oh!

justinmchase Sep 19, 2015

Choose a reason for hiding this comment

Uh oh!

justinmchase commented Sep 19, 2015

Uh oh!

Fishrock123 commented Sep 20, 2015

Uh oh!

justinmchase commented Sep 20, 2015

Uh oh!

justinmchase commented Sep 24, 2015

Uh oh!

justinmchase commented Sep 24, 2015

Uh oh!

Fishrock123 commented Sep 24, 2015

Uh oh!

justinmchase commented Sep 24, 2015

Uh oh!

bnoordhuis Sep 30, 2015

Choose a reason for hiding this comment

Uh oh!

justinmchase Sep 30, 2015

Choose a reason for hiding this comment

Uh oh!

thefourtheye Oct 2, 2015

Choose a reason for hiding this comment

Uh oh!

justinmchase Oct 2, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Sep 30, 2015

Uh oh!

thefourtheye commented Sep 30, 2015

Uh oh!

piscisaureus commented Sep 30, 2015

Uh oh!

justinmchase commented Sep 30, 2015

Uh oh!

benjamingr commented Sep 30, 2015

Uh oh!

bnoordhuis commented Oct 1, 2015

Uh oh!

orangemocha commented Oct 1, 2015

Uh oh!

justinmchase commented Oct 1, 2015

Uh oh!

justinmchase commented Oct 2, 2015

Uh oh!

bnoordhuis commented Oct 3, 2015

Uh oh!

Fishrock123 commented Oct 5, 2015

Uh oh!

bnoordhuis commented Oct 5, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lib,test: Use long paths when loading native addons#2965
justinmchase wants to merge 1 commit into
nodejs:masterfrom
justinmchase:use-long-path-to-load-addons