module,win: fix long path resolve #53294

huseyinacacak-janea · 2024-06-03T10:25:51Z

Several issues were encountered with module loading due to extended path lengths on Windows. These have been addressed with corrections implemented across four distinct code locations, accompanied by the addition of a corresponding test case for each to ensure functionality.
Additionally, I've moved es-module/test-GH-50753.js to es-module/test-esm-long-path-win.js as the tests cover more cases than specified in that issue.

Fixes: #50753

targos · 2024-06-03T12:11:39Z

@nodejs/platform-windows

I actually need more time to take a look, I think the implementation will solve the problem but I want to test in which cases we should include the UNC path, since we didn't do this in all paths in the toNamespacedPath.

H4ad · 2024-06-03T13:02:56Z

There are some conditions to include the UNC that you are not following:

node/lib/path.js

Lines 632 to 645 in 58711c2

    
           if (StringPrototypeCharCodeAt(resolvedPath, 0) === CHAR_BACKWARD_SLASH) { 
        
             // Possible UNC root 
        
             if (StringPrototypeCharCodeAt(resolvedPath, 1) === CHAR_BACKWARD_SLASH) { 
        
               const code = StringPrototypeCharCodeAt(resolvedPath, 2); 
        
               if (code !== CHAR_QUESTION_MARK && code !== CHAR_DOT) { 
        
                 // Matched non-long UNC root, convert the path to a long UNC path 
        
                 return `\\\\?\\UNC\\${StringPrototypeSlice(resolvedPath, 2)}`; 
        
               } 
        
             } 
        
           } else if ( 
        
             isWindowsDeviceRoot(StringPrototypeCharCodeAt(resolvedPath, 0)) && 
        
             StringPrototypeCharCodeAt(resolvedPath, 1) === CHAR_COLON && 
        
             StringPrototypeCharCodeAt(resolvedPath, 2) === CHAR_BACKWARD_SLASH 
        
           ) {

Although the condition could be matched, I think it would be better to wait for the migration of toNamespacedPath to C++.

Maybe we could include the ToNamespacePath port in this PR, and then leave that other PR just to remove the JS version and rewrite the places they are used.

@anonrig What do you think?

nodejs-github-bot · 2024-06-03T14:59:37Z

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

anonrig

I prefer to land this after the toNamespacedPath migration to C++. Unfortunately, I don't have the time to pursue this at the moment (afaict, there is only one failing test). I appreciate if you could took over that PR and apply these changes in a follow up PR.

MoLow · 2024-06-03T21:08:30Z

I prefer to land this after the toNamespacedPath migration to C++. Unfortunately, I don't have the time to pursue this at the moment (afaict, there is only one failing test). I appreciate if you could took over that PR and apply these changes in a follow up PR.

in that case, can you convert the blocking "request changes" into a simple suggestion/friendly ask? I don't think this PR should be blocked on a PR that is not being currently worked on

nodejs-github-bot · 2024-06-04T06:11:34Z

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

huseyinacacak-janea · 2024-06-05T06:34:00Z

I prefer to land this after the toNamespacedPath migration to C++. Unfortunately, I don't have the time to pursue this at the moment (afaict, there is only one failing test). I appreciate if you could took over that PR and apply these changes in a follow up PR.

Sure, no problem, I'll take over your PR and start working on it.

nodejs-github-bot · 2024-07-04T01:27:02Z

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

huseyinacacak-janea · 2024-07-05T12:48:14Z

I prefer to land this after the toNamespacedPath migration to C++. Unfortunately, I don't have the time to pursue this at the moment (afaict, there is only one failing test). I appreciate if you could took over that PR and apply these changes in a follow up PR.

@anonrig I've rebased this PR to use toNamespacedPath. Could you please review it?

huseyinacacak-janea · 2024-07-15T11:59:07Z

Is there anything else I can do to help this PR move forward?

anonrig · 2024-07-02T12:45:38Z

src/node_file.cc

-      switch (FilePathIsFile(env, file_path)) {
+      Local<Value> local_file_path =
+          Buffer::Copy(
+              env->isolate(), file_path.c_str(), strlen(file_path.c_str()))


Suggested change

env->isolate(), file_path.c_str(), strlen(file_path.c_str()))

env->isolate(), file_path.c_str(), file_path.size())

Fixed. Thanks.

anonrig · 2024-07-15T12:01:21Z

src/node_file.cc

-    switch (FilePathIsFile(env, file_path)) {
+    Local<Value> local_file_path =
+        Buffer::Copy(
+            env->isolate(), file_path.c_str(), strlen(file_path.c_str()))


anonrig · 2024-07-15T12:02:06Z

src/node_modules.cc

+  // Check if the path has a trailing slash. If so, add it after
+  // ToNamespacedPath() as it will be deleted by ToNamespacedPath()
+  bool slashCheck = !path_value.ToString().empty() &&
+                    path_value.ToString().back() ==


Suggested change

path_value.ToString().back() ==

path_value.ToStringView().ends_with

anonrig · 2024-07-15T12:02:31Z

src/node_modules.cc

+  // Check if the path has a trailing slash. If so, add it after
+  // ToNamespacedPath() as it will be deleted by ToNamespacedPath()
+  bool slashCheck = !path_value.ToString().empty() &&
+                    path_value.ToString().back() ==


anonrig · 2024-07-15T14:42:36Z

src/node_modules.cc

@@ -323,8 +323,8 @@ void BindingData::GetNearestParentPackageJSON(
  // Check if the path has a trailing slash. If so, add it after
  // ToNamespacedPath() as it will be deleted by ToNamespacedPath()
  bool slashCheck = !path_value.ToString().empty() &&
-                    path_value.ToString().back() ==
-                        std::filesystem::path::preferred_separator;
+                    path_value.ToStringView().ends_with(


The previous line is not needed path_value.ToString() creates an unnecessary string.

Can you also remove this in other places?

anonrig · 2024-07-15T14:43:45Z

src/node_file.cc

@@ -3096,8 +3096,14 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo<Value>& args) {

    for (int i = 0; i < legacy_main_extensions_with_main_end; i++) {
      file_path = *initial_file_path + std::string(legacy_main_extensions[i]);
-
-      switch (FilePathIsFile(env, file_path)) {
+      Local<Value> local_file_path =


Can you add a TODO here?

// TODO: Remove this when ToNamespacedPath supports std::string as a value

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 3, 2024

huseyinacacak-janea force-pushed the huseyin-11801-long-path-error branch from 3ee742c to 27a4505 Compare June 3, 2024 12:11

H4ad previously approved these changes Jun 3, 2024

View reviewed changes

StefanStojanovic added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2024

anonrig requested changes Jun 3, 2024

View reviewed changes

huseyinacacak-janea force-pushed the huseyin-11801-long-path-error branch from 27a4505 to 42b88bf Compare July 1, 2024 09:31

module,win: fix long path resolve

14a1de0

huseyinacacak-janea force-pushed the huseyin-11801-long-path-error branch from 42b88bf to 14a1de0 Compare July 2, 2024 05:37

StefanStojanovic added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 2, 2024

H4ad approved these changes Jul 2, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 3, 2024

H4ad requested a review from anonrig July 15, 2024 12:01

anonrig reviewed Jul 15, 2024

View reviewed changes

fixup! module,win: fix long path resolve

dae0bfe

anonrig reviewed Jul 15, 2024

View reviewed changes

fixup! module,win: fix long path resolve

0e2c739

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module,win: fix long path resolve #53294

module,win: fix long path resolve #53294

huseyinacacak-janea commented Jun 3, 2024

targos commented Jun 3, 2024

H4ad commented Jun 3, 2024

nodejs-github-bot commented Jun 3, 2024

anonrig left a comment

MoLow commented Jun 3, 2024

nodejs-github-bot commented Jun 4, 2024

huseyinacacak-janea commented Jun 5, 2024

nodejs-github-bot commented Jul 4, 2024

huseyinacacak-janea commented Jul 5, 2024

huseyinacacak-janea commented Jul 15, 2024

anonrig Jul 2, 2024

huseyinacacak-janea Jul 15, 2024

anonrig Jul 15, 2024

huseyinacacak-janea Jul 15, 2024

anonrig Jul 15, 2024

huseyinacacak-janea Jul 15, 2024

anonrig Jul 15, 2024

huseyinacacak-janea Jul 15, 2024

anonrig Jul 15, 2024

huseyinacacak-janea Jul 16, 2024

anonrig Jul 15, 2024

huseyinacacak-janea Jul 16, 2024

	env->isolate(), file_path.c_str(), strlen(file_path.c_str()))
	env->isolate(), file_path.c_str(), file_path.size())

	path_value.ToString().back() ==
	path_value.ToStringView().ends_with

module,win: fix long path resolve #53294

Are you sure you want to change the base?

module,win: fix long path resolve #53294

Conversation

huseyinacacak-janea commented Jun 3, 2024

targos commented Jun 3, 2024

H4ad commented Jun 3, 2024

nodejs-github-bot commented Jun 3, 2024

anonrig left a comment

Choose a reason for hiding this comment

MoLow commented Jun 3, 2024

nodejs-github-bot commented Jun 4, 2024

huseyinacacak-janea commented Jun 5, 2024

nodejs-github-bot commented Jul 4, 2024

huseyinacacak-janea commented Jul 5, 2024

huseyinacacak-janea commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment