path: fix bugs and inconsistencies #54224

huseyinacacak-janea · 2024-08-06T12:03:37Z

This PR fixes resolving device paths like \\.\PHYSICALDRIVE.
The resolve function was adding a trailing backslash, considering that this device path was a UNC path. However, device paths are different than UNC paths.

This PR includes more changes than originally intended because fixing the bug caused inconsistencies between other functions and operating systems. Now, this PR fixes all these problems.

Fixes: #54025

codecov · 2024-08-06T13:16:01Z

Codecov Report

Attention: Patch coverage is 90.32258% with 12 lines in your changes missing coverage. Please review.

Project coverage is 87.89%. Comparing base (b4fd1fd) to head (34839d8).
Report is 441 commits behind head on main.

Files with missing lines	Patch %	Lines
src/path.cc	38.46%	6 Missing and 2 partials ⚠️
lib/path.js	95.91%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54224      +/-   ##
==========================================
+ Coverage   87.06%   87.89%   +0.83%     
==========================================
  Files         643      651       +8     
  Lines      181576   183425    +1849     
  Branches    34894    35744     +850     
==========================================
+ Hits       158088   161222    +3134     
+ Misses      16759    15476    -1283     
+ Partials     6729     6727       -2

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	`96.19% <100.00%> (+3.26%)`	⬆️
lib/internal/url.js	`98.70% <ø> (+3.93%)`	⬆️
lib/path.js	`97.27% <95.91%> (+1.33%)`	⬆️
src/path.cc	`67.39% <38.46%> (-1.66%)`	⬇️

... and 206 files with indirect coverage changes

lpinca · 2024-08-06T15:38:07Z

lib/path.js

+                    `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`;
+                  rootEnd = j;
+                } else {
+                  // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0)


I think it should also handle \\?\PHYSICALDRIVE0.

Thanks for the review. Fixed.

There are other inconsistencies:

path.resolve('\\\\?\\c:/') // \\?\c: path.toNamespacedPath('c:/') // \\?\c:\ path.normalize('\\\\.\\foo') // \\.\foo\ path.resolve('\\\\.\\foo\\') // \\.\foo

OK, I'll try to fix these inconsistencies. I have a question.
Node currently runs as follows:

path.resolve("C:\\") // C:\\ path.resolve("//server/share") // \\\\server\\share\\

But, I think it should be:

path.resolve("C:\\") // C: path.resolve("//server/share") // \\\\server\\share

If I change these ones, resolve could return all the paths without any trailing backslash.
If I don't change them, the inconsistency below will still exist:

path.resolve("//server/share/folder") // \\\\server\\share\\folder path.resolve("//server/share") // \\\\server\\share\\

What do you think?

I think it should respect the original input. If there is a trailing slash, keep it, otherwise do not add it.

$ python Python 3.12.4 (tags/v3.12.4:8e8a4ba, Jun 6 2024, 19:30:16) [MSC v.1940 64 bit (AMD64)] on win32 Type "help", "copyright", "credits" or "license" for more information. >>> from pathlib import PurePath >>> PurePath('c:/') PureWindowsPath('c:/') >>> PurePath('c:') PureWindowsPath('c:') >>> PurePath('c:', '\\\\.\\foo') PureWindowsPath('//./foo') >>> PurePath('c:', '\\\\.\\foo\\') PureWindowsPath('//./foo/') >>>

AFAIK \\?\... is not supposed to be subject to normalization, see this post.

My latest commit fixes the inconsistencies in resolve, toNamespacedPath and normalize on Windows. I'm open to suggestions and waiting for your reviews.
If there are no objections, I'll fix the inconsistencies between Windows and Posix that occurred after the last commit.

Fixes: nodejs#54025

huseyinacacak-janea · 2024-08-21T12:02:17Z

Is there anything else I can do to help this PR move forward?
Note that since this PR includes a lot of breaking changes, I think it would be better to label this as semver-major.

lpinca · 2024-08-21T12:25:12Z

@nodejs/path @nodejs/tsc

ShenHongFei · 2024-09-19T18:04:38Z

@huseyinacacak-janea @anonrig @lpinca

This change modifies the return results of path.resolve and path.relative, which will cause many third-party libraries to suddenly stop working. Please carefully consider whether you really want to change this way.
I encountered an error with npm publish. The main reason for the impact is that npm's processing of paths depends on resolve and relative functions, which will automatically remove the trailing /

npm publish error log

0 verbose cli D:\1\nodejs\node.exe D:\1\nodejs\node_modules\npm\bin\npm-cli.js
1 info using [email protected]
2 info using [email protected]
3 silly config load:file:D:\1\nodejs\node_modules\npm\npmrc
4 silly config load:file:D:\1\xsh\.npmrc
5 silly config load:file:C:\Users\shf\.npmrc
6 silly config load:file:D:\1\nodejs\etc\npmrc
7 verbose title npm publish
8 verbose argv "publish" "--proxy" "http://127.0.0.1:10080" "--registry" "https://registry.npmjs.org/"
9 verbose logfile logs-max:10 dir:T:\t\npm-cache\_logs\2024-09-19T16_52_55_988Z-
10 verbose logfile T:\t\npm-cache\_logs\2024-09-19T16_52_55_988Z-debug-0.log
11 verbose publish [ '.' ]
12 silly logfile start cleaning logs, removing 1 files
13 silly logfile done cleaning log files
14 verbose stack Error: ENOENT: no such file or directory, open 'D:\1\README.md'
14 verbose stack     at async open (node:internal/fs/promises:639:25)
14 verbose stack     at async Object.readFile (node:internal/fs/promises:1239:14)
14 verbose stack     at async normalize (D:\1\nodejs\node_modules\npm\node_modules\@npmcli\package-json\lib\normalize.js:348:26)
14 verbose stack     at async PackageJson.prepare (D:\1\nodejs\node_modules\npm\node_modules\@npmcli\package-json\lib\index.js:266:5)
14 verbose stack     at async #getManifest (D:\1\nodejs\node_modules\npm\lib\commands\publish.js:209:27)
14 verbose stack     at async #publish (D:\1\nodejs\node_modules\npm\lib\commands\publish.js:93:20)
14 verbose stack     at async Publish.exec (D:\1\nodejs\node_modules\npm\lib\commands\publish.js:46:5)
14 verbose stack     at async Npm.exec (D:\1\nodejs\node_modules\npm\lib\npm.js:207:9)
14 verbose stack     at async module.exports (D:\1\nodejs\node_modules\npm\lib\cli\entry.js:74:5)
15 error code ENOENT
16 error syscall open
17 error path D:\1\README.md
18 error errno -4058
19 error enoent ENOENT: no such file or directory, open 'D:\1\README.md'
20 error enoent This is related to npm not being able to find a file.
20 error enoent
21 verbose cwd D:\1\xsh
22 verbose os Windows_NT 10.0.22631
23 verbose node v23.0.0
24 verbose npm  v10.8.3
25 verbose exit -4058
26 verbose code -4058
27 error A complete log of this run can be found in: T:\t\npm-cache\_logs\2024-09-19T16_52_55_988Z-debug-0.log

The relevant code for the error in npm is as follows:

https://github.com/npm/npm-package-arg/blob/a8a9bddc726802fb5ed30f6b113d57f0655bbd51/lib/npa.js#L296

The reason for the error is that the following function returns inconsistent results:

// main branch
path.resolve('D:\\1\\xsh\\') === 'D:\\1\\xsh\\'
path.relative('D:\\1\\xsh', 'D:\\1\\xsh\\') === '..\\xsh'

// node.js v22.9.0
path.resolve('D:\\1\\xsh\\') === 'D:\\1\\xsh'
path.relative('D:\\1\\xsh', 'D:\\1\\xsh\\') === ''

lpinca · 2024-09-19T18:20:11Z

Ok, if it too breaking of a change, I think we should revert efbba60 and reopen #54025.

path.relative('D:\\1\\xsh', 'D:\\1\\xsh\\') === '..\\xsh'

The above result is weird.

lpinca · 2024-09-19T19:20:40Z

Maybe we can limit the scope and go back to b40e78c.

avivkeller · 2024-10-17T14:29:33Z

This did not fix that issue, in fact, the fix is a revert of this #55414

vzaidman · 2024-10-17T14:33:39Z

This did not fix that issue, in fact, the fix is a revert of this #55414

oops, commented on the wrong PR. deleted the comment to not create confusion.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. labels Aug 6, 2024

anonrig approved these changes Aug 6, 2024

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2024

avivkeller added the windows Issues and PRs related to the Windows platform. label Aug 6, 2024

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

This comment was marked as outdated.

Sign in to view

anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 6, 2024

lpinca reviewed Aug 6, 2024

View reviewed changes

huseyinacacak-janea force-pushed the huseyin-12382-physicaldrive-open-is-broken branch from b40e78c to ab4ad3b Compare August 7, 2024 08:35

lpinca approved these changes Aug 12, 2024

View reviewed changes

huseyinacacak-janea force-pushed the huseyin-12382-physicaldrive-open-is-broken branch 2 times, most recently from 888faf2 to 69c609d Compare August 15, 2024 06:02

targos mentioned this pull request Aug 15, 2024

path: update win32 toNamespacedPath to support device namespace paths #54367

Open

huseyinacacak-janea force-pushed the huseyin-12382-physicaldrive-open-is-broken branch 2 times, most recently from 4e2dddd to 1601604 Compare August 15, 2024 08:38

path: fix bugs and inconsistencies

a5540cb

Fixes: nodejs#54025

huseyinacacak-janea force-pushed the huseyin-12382-physicaldrive-open-is-broken branch from 1601604 to a5540cb Compare August 16, 2024 12:46

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2024

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

This comment was marked as outdated.

Sign in to view

lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 21, 2024

lpinca changed the title ~~path,win: fix bug in resolve~~ path: fix bugs and inconsistencies Aug 22, 2024

zcbenz mentioned this pull request Sep 23, 2024

src: do not call path.back() when it is empty #55072

Merged

RafaelGSS mentioned this pull request Oct 9, 2024

2024-10-16, Version 23.0.0 (Current) #55338

Merged

richardlau mentioned this pull request Oct 17, 2024

npm pack crash in Node 23 #55410

Closed

lpinca mentioned this pull request Oct 17, 2024

Revert "path: fix bugs and inconsistencies" #55414

Merged

fasttime mentioned this pull request Oct 18, 2024

Bug: incorrect file enumeration with legacy config in Node.js 23 eslint/eslint#19031

Closed

1 task

aladdin-add mentioned this pull request Oct 22, 2024

ci: Add node 23 to ci workflows eslint-community/eslint-plugin-n#368

Merged

scagood mentioned this pull request Oct 24, 2024

Regression when updating to node: ^23 (no-unpublished-require) eslint-community/eslint-plugin-n#369

Closed

clemyan mentioned this pull request Oct 25, 2024

Use Node 22 for GitHub actions for now yarnpkg/berry#6575

Merged

3 tasks

huseyinacacak-janea mentioned this pull request Oct 31, 2024

path,win: fix bug in resolve and normalize #55623

Merged

Flarna mentioned this pull request Nov 30, 2024

Revert of nodejs/node#55623 #56145

Closed

joyeecheung mentioned this pull request Jan 23, 2025

[v20.x backport] src: move package_json_reader cache to c++ #56590

Closed

Uh oh!

path: fix bugs and inconsistencies #54224

path: fix bugs and inconsistencies #54224

Uh oh!

Conversation

huseyinacacak-janea commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

codecov bot commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lpinca Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huseyinacacak-janea Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

lpinca Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huseyinacacak-janea Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

lpinca Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

huseyinacacak-janea Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

huseyinacacak-janea commented Aug 21, 2024

Uh oh!

lpinca commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShenHongFei commented Sep 19, 2024

Uh oh!

lpinca commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Sep 19, 2024

Uh oh!

avivkeller commented Oct 17, 2024

Uh oh!

vzaidman commented Oct 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

huseyinacacak-janea commented Aug 6, 2024 •

edited

Loading

codecov bot commented Aug 6, 2024 •

edited

Loading

lpinca Aug 6, 2024 •

edited

Loading

lpinca Aug 7, 2024 •

edited

Loading

lpinca Aug 7, 2024 •

edited

Loading

huseyinacacak-janea Aug 12, 2024 •

edited

Loading

lpinca commented Aug 21, 2024 •

edited

Loading

lpinca commented Sep 19, 2024 •

edited

Loading