FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module,win: fix long path resolve by StefanStojanovic · Pull Request #51097 · nodejs/node · GitHub

/ node Public

module,win: fix long path resolve - #51097

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
JaneaSystems:mefi-resolve-long-path
Apr 8, 2024
Merged

module,win: fix long path resolve#51097
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
JaneaSystems:mefi-resolve-long-path

Conversation

Copy link
Copy Markdown
Contributor

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

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 Dec 8, 2023

anonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

FYI @nodejs/loaders @nodejs/fs

Copy link
Copy Markdown
Contributor Author

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

Of course. I'll add it next week.

Copy link
Copy Markdown
Contributor Author

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

Test's added.

joyeecheung left a comment
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

Copy link
Copy Markdown
Contributor Author

@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?

Copy link
Copy Markdown
Member

Yes, with a TODO/FIXME comment.

Copy link
Copy Markdown
Contributor Author

@joyeecheung LGTY now?

StefanStojanovic force-pushed the mefi-resolve-long-path branch 3 times, most recently from e72e49a to 4e651a1 Compare February 9, 2024 13:22
joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 1, 2024

Copy link
Copy Markdown
Collaborator

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 1, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 1, 2024

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Contributor Author

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

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024

Copy link
Copy Markdown
Collaborator

karlhorky commented Apr 3, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

@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.

zkochan mentioned this pull request Apr 7, 2024
11 tasks
joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024
nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 8, 2024

Copy link
Copy Markdown
Collaborator
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

Copy link
Copy Markdown
Contributor Author

Commit Queue failed


⚠ 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?

joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 8, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 8, 2024

Copy link
Copy Markdown
Collaborator

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2024
nodejs-github-bot merged commit 45f0dd0 into nodejs:main Apr 8, 2024

Copy link
Copy Markdown
Collaborator

Landed in 45f0dd0

Copy link
Copy Markdown
Contributor

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

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)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Long node_modules paths cannot be found on Windows when LongPathsEnabled enabled

8 participants


Back | FazBrowse Home | New Git URL