module,win: fix long path resolve

[StefanStojanovic]

As reported here, there is a problem loading modules from long paths on Windows. This change fixes it by adding the required \\?\ prefix, with which, resolving long paths will work regardless of whether the LongPathsEnabled value is set or not.

Fixes: #50753

[anonrig]

Can we add a test so we don't regress?

[richardlau]

FYI @nodejs/loaders @nodejs/fs

[StefanStojanovic]

Can we add a test so we don't regress?

Of course. I'll add it next week.

[StefanStojanovic]

Can we add a test so we don't regress?

Test's added.

[joyeecheung]

This looks like a regression from #50322, probably from not porting the logic in path.toNamespacedPath() properly

node/lib/path.js

Lines 632 to 648 in b7def8e

	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
	) {
	// Matched device root, convert the path to a long UNC path
	return `\\\\?\\${resolvedPath}`;
	}

We should probably port the entire logic over (with a helper that's named something like ToNamespacedPath declared in either util.h or node_internals.h)

[StefanStojanovic]

@joyeecheung do you think this can land as a fix with a regression test added and then a follow-up PR can be opened for porting path.toNamespacedPath() logic and using it in here, but potentially in other places missing it as well?

[joyeecheung]

Yes, with a TODO/FIXME comment.

[StefanStojanovic]

@joyeecheung LGTY now?

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[StefanStojanovic]

@joyeecheung can you add the commit-queue label to try to land this again? Last time it failed to do so...

[nodejs-github-bot]

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

[karlhorky]

@StefanStojanovic @joyeecheung any chance of getting this released in one of the next patch or minor releases?

It's been a while since my original issue in Nov 2023 and users keep running into this (eg. with pnpm) and commenting that it's an open bug but the PR hasn't been merged yet.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/51097
✔  Done loading data for nodejs/node/pull/51097
----------------------------------- PR info ------------------------------------
Title      module,win: fix long path resolve (#51097)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     StefanStojanovic:mefi-resolve-long-path -> nodejs:main
Labels     c++, fs, needs-ci
Commits    1
 - module,win: fix long path resolve
Committers 1
 - StefanStojanovic 
PR-URL: https://github.com/nodejs/node/pull/51097
Fixes: https://github.com/nodejs/node/issues/50753
Reviewed-By: Joyee Cheung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51097
Fixes: https://github.com/nodejs/node/issues/50753
Reviewed-By: Joyee Cheung 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - module,win: fix long path resolve
   ℹ  This PR was created on Fri, 08 Dec 2023 13:28:00 GMT
   ✔  Approvals: 1
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/51097#pullrequestreview-1891732903
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-04-01T17:51:22Z: https://ci.nodejs.org/job/node-test-pull-request/58036/
- Querying data for job/node-test-pull-request/58036/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/8596048782

[StefanStojanovic]

Commit Queue failed

• Loading data for module,win: fix long path resolve #51097
  ✔ Done loading data for module,win: fix long path resolve #51097
  ----------------------------------- PR info ------------------------------------
  Title module,win: fix long path resolve (module,win: fix long path resolve #51097)
  ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
  Branch StefanStojanovic:mefi-resolve-long-path -> nodejs:main
  Labels c++, fs, needs-ci
  Commits 1
• module,win: fix long path resolve
  Committers 1
• StefanStojanovic
  PR-URL: module,win: fix long path resolve #51097
  Fixes: Long node_modules paths cannot be found on Windows when LongPathsEnabled enabled #50753
  Reviewed-By: Joyee Cheung
  ------------------------------ Generated metadata ------------------------------
  PR-URL: module,win: fix long path resolve #51097
  Fixes: Long node_modules paths cannot be found on Windows when LongPathsEnabled enabled #50753
  Reviewed-By: Joyee Cheung

---

⚠ Commits were pushed since the last approving review:
⚠ - module,win: fix long path resolve
ℹ This PR was created on Fri, 08 Dec 2023 13:28:00 GMT
✔ Approvals: 1
✔ - Joyee Cheung (@joyeecheung) (TSC): #51097 (review)
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2024-04-01T17:51:22Z: https://ci.nodejs.org/job/node-test-pull-request/58036/

• Querying data for job/node-test-pull-request/58036/
  ✔ Last Jenkins CI successful

---

✔ Aborted git node land session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/8596048782

@joyeecheung do you see why this couldn't land from this log?

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 45f0dd0

[karlhorky]

It looks like the Node.js v22 PR contains the fix "module,win: fix long path resolve (by @StefanStojanovic) #51097":

• 2024-04-24, Version 22.0.0 (Current) #52505

So maybe Node.js has support for long paths on Windows now!

(make sure that you have LongPathsEnabled set to true on the Windows machine you test on)