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

require fails to load 'package.json' when the module path contains Chinese characters on Windows. #2236

Closed
hokein opened this issue Jul 24, 2015 · 16 comments
Labels
confirmed-bug Issues with confirmed bugs. i18n-api Issues and PRs related to the i18n implementation. module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform.

Comments

@hokein
Copy link
Contributor

hokein commented Jul 24, 2015

Reproduce steps:

  • Create a Chinese-name node module directory that contains a package.json, like
 中文目录
  |-- main.js
  |-- package.json

And the content of package.json:

{
  "name": "test",
  "productName": "test",
  "main": "main.js"
}
  • Type require("<Chinese-path>") in iojs, then Error Error: Cannot find module XXX will arise.

It fails on iojs v2.4 and v3.0 rc4, but works on v1.6, I believe a regression issue. What's more, it seems only fails to read the package.json file. If we require a absolute js path(<Chinese-path>/main.js), it loads the module well.

@hokein hokein changed the title require fails to load 'package.json' when the module path contains Chinese characters. require fails to load 'package.json' when the module path contains Chinese characters on Windows. Jul 24, 2015
@mscdex mscdex added module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform. labels Jul 24, 2015
@targos
Copy link
Member

targos commented Aug 4, 2015

just confirmed. The regression appeared with v2.2.0

@targos targos added the confirmed-bug Issues with confirmed bugs. label Aug 4, 2015
@targos
Copy link
Member

targos commented Aug 4, 2015

The module resolution fails because it cannot read the package.json file.
I suspect a problem with internalModuleReadFile and unicode paths on Windows.
/cc @bnoordhuis

@bpasero
Copy link
Contributor

bpasero commented Aug 13, 2015

@bnoordhuis @trevnorris this is really bad and a huge regression from pre 2.2.0 io.js. I can reproduce the require call to fail easily by having a index.js file sit inside a folder with german umlauts (or any other non ascii character).

This might be less critical for use cases of io.js in server environments and Mac/Linux, but dont forget that e.g. Electron (the shell of the atom editor: https://github.com/atom/electron) is using io.js for all node matters and you can easily end up installing it on a Windows path with special characters.

From looking at the code, it seems to fail inside InternalModuleReadFile (https://github.com/bnoordhuis/io.js/blob/master/src/node_file.cc#L443), maybe in the fopen call. That is also the only place where fopen is being used. Other fs calls like the internal stat call are not failing like that.

@trevnorris
Copy link
Contributor

@bnoordhuis Should Windows use _wfopen() instead?

@bnoordhuis
Copy link
Member

I'd use uv_fs_open() + uv_fs_read().

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Aug 13, 2015
@brendanashworth
Copy link
Contributor

This does seem to be rather bad, I'd consider this a candidate for blocking the 3.1 release, thoughts?

@bnoordhuis
Copy link
Member

It's not a regression in v3.x (because it's older than that) but fixing it of course won't hurt: #2377

@bpasero
Copy link
Contributor

bpasero commented Aug 14, 2015

Thanks for the prompt fix!

@bnoordhuis
Copy link
Member

I need a volunteer that's affected by this to test the changes in #2377. The current working hypothesis is that it's related to the code page.

@bpasero
Copy link
Contributor

bpasero commented Aug 16, 2015

@bnoordhuis I verified that your change fixes the issue for me. My test case is this:

  • have a simple index.js that requires a module (used iconv-lite in this case)
  • npm install the module into the folder of the index.js
  • name the parent folder of the index.js with non ascii characters ("fooäbar")
  • run "iojs index"

With your fix I can load the module and before I could not.

I am not sure about the theory this is code page related. My windows installation is set to english locale. Please let me know if you need more infos of my windows installation to reproduce. I can also attach my sample to see if you can repro.

@bnoordhuis
Copy link
Member

@bpasero Thanks for testing. What about the regression test from #2377? Does it pass or fail for you without the fix?

@bpasero
Copy link
Contributor

bpasero commented Aug 17, 2015

@bnoordhuis it fails for me without the fix and passes with. Failing output:

C:\GitDevelopment\iojs-bnoordhuis\Release>iojs.exe C:\GitDevelopment\iojs-bnoordhuis\test\parallel\test-require-unicode.js
module.js:338
throw err;
^

Error: Cannot find module 'C:\GitDevelopment\iojs-bnoordhuis\test\tmp\õ©¡µûçþø«Õ¢ò'
at Function.Module._resolveFilename (module.js:336:15)
at Function.Module._load (module.js:286:25)
at Module.require (module.js:365:17)
at require (module.js:384:17)
at Object. (C:\GitDevelopment\iojs-bnoordhuis\test\parallel\test-require-unicode.js:15:14)
at Module._compile (module.js:430:26)
at Object.Module._extensions..js (module.js:448:10)
at Module.load (module.js:355:32)
at Function.Module._load (module.js:310:12)
at Function.Module.runMain (module.js:471:10)

@bpasero
Copy link
Contributor

bpasero commented Aug 17, 2015

image

@bnoordhuis
Copy link
Member

Okay, thanks. Perhaps it's a compatibility issue with newer Windows versions. Our CI consists of Windows Server 2008 and 2012.

@silverwind
Copy link
Contributor

@bnoordhuis from what I gather, this should get you utf8 mode fopen:

fopen(file, "r+, ccs=UTF-8")

With unspecified encoding, the system should fall back to the user's locale. Maybe that fallback behaviour has changed in recent Windows versions.

https://msdn.microsoft.com/en-us/library/yeby3zcb.aspx
http://stackoverflow.com/a/2051312/808699

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Aug 17, 2015
Fix a regression that was introduced in commit 1bbf8d0 ("lib: speed up
require(), phase 2") where file paths with Unicode characters fail to
load on Windows.

Fixes: nodejs#2236
PR-URL: nodejs#2377
Reviewed-By: Bert Belder <[email protected]>
@bnoordhuis
Copy link
Member

Should be fixed by a593cb7, /cc @Fishrock123.

Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Aug 19, 2015
Fix a regression that was introduced in commit 1bbf8d0 ("lib: speed up
require(), phase 2") where file paths with Unicode characters fail to
load on Windows.

Fixes: nodejs#2236
PR-URL: nodejs#2377
Reviewed-By: Bert Belder <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. i18n-api Issues and PRs related to the i18n implementation. module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

9 participants