path,win: fix bug in resolve and normalize by huseyinacacak-janea · Pull Request #55623 · nodejs/node

huseyinacacak-janea · 2024-10-31T13:47:57Z

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

In addition to fixing this issue, the previous attempt also fixed other inconsistencies. So, this PR only fixes the issue itself.

Previous PR: #54224
Fixes: #54025

Fixes: nodejs#54025

avivkeller · 2024-10-31T13:55:23Z

test/parallel/test-path-makelong.js

 assert.strictEqual(path.win32.toNamespacedPath('//foo//bar'),
                   '\\\\?\\UNC\\foo\\bar\\');
-assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo\\');
+assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo');


Is stripping the / okay? We wouldn't want to repeat that happened with npm 😅

I think it should be OK since this PR doesn't change anything other than this particular path format. But yes, it would be useful to see the citgm test results.

Yes, a CITGM would be nice. I've applied the labels. If possible, could you also run npm's test suite? The CITGM doesn't cover all of it, and IMO we should be extra cautious JIC.

Although, it seems like it should be fine. Just want to confirm

I ran the test in the npm repo locally and didn't encounter any problems.

codecov · 2024-10-31T15:28:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.43%. Comparing base (9967080) to head (efe96fc).
⚠️ Report is 2276 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #55623   +/-   ##
=======================================
  Coverage   88.43%   88.43%           
=======================================
  Files         654      654           
  Lines      187698   187729   +31     
  Branches    36126    36135    +9     
=======================================
+ Hits       165986   166024   +38     
  Misses      14953    14953           
+ Partials     6759     6752    -7

Files with missing lines	Coverage Δ
lib/path.js	`96.14% <100.00%> (+0.14%)`	⬆️
src/path.cc	`69.04% <ø> (ø)`

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nodejs-github-bot · 2024-11-01T08:23:26Z

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

aduh95 · 2024-11-03T11:04:51Z

CITGM (main): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3499/
CITGM (this PR): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3501/

SUCCESS: No new failures in 3501 compared to 3499 🎉

nodejs-github-bot · 2024-11-05T23:21:55Z

Landed in 7f68e54

Fixes: #54025 PR-URL: #55623 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Fixes: nodejs#54025 PR-URL: nodejs#55623 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

PR-URL: #56110 Fixes: #56002 Refs: #55623 Refs: #56088 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Flarna · 2024-12-23T15:12:47Z

removing the don't land labels since #56002 was fixed via #56110 which was released in 23.5.0.

PR-URL: #56110 Fixes: #56002 Refs: #55623 Refs: #56088 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

aduh95 · 2025-02-04T11:06:22Z

I had to drop it from 22.14.0 proposal as the following test was failing:

parallel/test-path-normalize

=== release test-path-normalize ===                                           
Path: parallel/test-path-normalize
node:assert:128
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '\\\\?\\?\\D:\\Test'
- '\\\\?\\test\\?\\D:\\Test'
          ^

    at Object.<anonymous> (…/test/parallel/test-path-normalize.js:63:8)
    at Module._compile (node:internal/modules/cjs/loader:1555:14)
    at Object..js (node:internal/modules/cjs/loader:1707:10)
    at Module.load (node:internal/modules/cjs/loader:1290:32)
    at Function._load (node:internal/modules/cjs/loader:1109:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:220:24)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:170:5)
    at node:internal/main/run_main_module:36:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '\\\\?\\?\\D:\\Test',
  expected: '\\\\?\\test\\?\\D:\\Test',
  operator: 'strictEqual'
}

Node.js v22.13.2-pre

Manual backport is needed if we want it on v22.x.

Fixes: nodejs#54025 PR-URL: nodejs#55623 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Fixes: #54025 PR-URL: #55623 Backport-PR-URL: #59831 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

This fixes local flash (and all other flashes) on windows machines after a bugfix in a issue coming from nodejs See: nodejs/node#55623 See: #2959 Change-type: patch

Fixes: #54025 PR-URL: #55623 Backport-PR-URL: #59831 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

- downgrade flasher's node to 20.11.1 on windows - bump windows GHA runner to 2022 - bump winusb-driver-generator to 2.1.9

path,win: fix bug in resolve and normalize

efe96fc

Fixes: nodejs#54025

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 Oct 31, 2024

avivkeller added the needs-citgm PRs that need a CITGM CI run. label Oct 31, 2024

avivkeller reviewed Oct 31, 2024

View reviewed changes

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

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

aduh95 approved these changes Nov 4, 2024

View reviewed changes

BridgeAR approved these changes Nov 4, 2024

View reviewed changes

lpinca approved these changes Nov 5, 2024

View reviewed changes

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Nov 5, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 5, 2024

nodejs-github-bot merged commit 7f68e54 into nodejs:main Nov 5, 2024

aduh95 mentioned this pull request Nov 6, 2024

2024-11-11, Version 23.2.0 (Current) #55741

Merged

Flarna mentioned this pull request Nov 29, 2024

Node 23.2 fs lost the ability to enumerate named pipes on Windows #56002

Closed

Flarna mentioned this pull request Dec 2, 2024

fs,win: fix readdir for named pipe #56110

Merged

nodejs-github-bot pushed a commit that referenced this pull request Dec 10, 2024

fs,win: fix readdir for named pipe

f184a0a

PR-URL: #56110 Fixes: #56002 Refs: #55623 Refs: #56088 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

aduh95 pushed a commit that referenced this pull request Dec 10, 2024

fs,win: fix readdir for named pipe

595851b

PR-URL: #56110 Fixes: #56002 Refs: #55623 Refs: #56088 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

ruyadorno pushed a commit that referenced this pull request Dec 20, 2024

fs,win: fix readdir for named pipe

437021d

PR-URL: #56110 Fixes: #56002 Refs: #55623 Refs: #56088 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Flarna removed dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Dec 23, 2024

ruyadorno pushed a commit that referenced this pull request Jan 5, 2025

fs,win: fix readdir for named pipe

0331b3f

PR-URL: #56110 Fixes: #56002 Refs: #55623 Refs: #56088 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Flarna mentioned this pull request Jan 7, 2025

2025-01-07, Version 22.13.0 'Jod' (LTS) #56329

Merged

aduh95 removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Jan 7, 2025

aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Feb 4, 2025

otaviojacobi mentioned this pull request Jul 22, 2025

Balena local flash failing balena-io/balena-cli#2959

Closed

aduh95 mentioned this pull request Aug 26, 2025

lib: handle windows reserved device names on UNC #59286

Merged

RafaelGSS mentioned this pull request Sep 9, 2025

[v22.x backport] lib: handle windows reserved device names on UNC #59831

Closed

RafaelGSS added backport-open-v22.x Indicate that the PR has an open backport and removed backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Sep 9, 2025

richardlau added backported-to-v22.x PRs backported to the v22.x-staging branch. and removed backport-open-v22.x Indicate that the PR has an open backport labels Sep 17, 2025

github-actions bot mentioned this pull request Sep 22, 2025

2025-09-24, Version 22.20.0 'Jod' (LTS) #59973

Merged

Flarna mentioned this pull request Sep 24, 2025

CONOUT$ no longer available for Node 22? #54161

Closed

2 tasks

otaviojacobi mentioned this pull request Oct 8, 2025

Build with node 22.20.0 balena-io/balena-cli#2999

Merged

marco-ippolito mentioned this pull request Mar 3, 2026

2026-03-05, Version 20.20.1 'Iron' (LTS), @marco-ippolito #62088

Merged

sul90tan referenced this pull request in balena-io/etcher Mar 4, 2026

patch: fix windows build and flash issues

2a470f5

- downgrade flasher's node to 20.11.1 on windows - bump windows GHA runner to 2022 - bump winusb-driver-generator to 2.1.9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

path,win: fix bug in resolve and normalize#55623
nodejs-github-bot merged 1 commit intonodejs:mainfrom
JaneaSystems:huseyin-12648-physical-drive-broken

huseyinacacak-janea commented Oct 31, 2024

Uh oh!

avivkeller Oct 31, 2024

Uh oh!

huseyinacacak-janea Oct 31, 2024

Uh oh!

avivkeller Oct 31, 2024 •

edited

Loading

Uh oh!

huseyinacacak-janea Nov 4, 2024

Uh oh!

codecov bot commented Oct 31, 2024 •

edited

Loading

Uh oh!

nodejs-github-bot commented Nov 1, 2024

Uh oh!

aduh95 commented Nov 3, 2024 •

edited

Loading

Uh oh!

nodejs-github-bot commented Nov 5, 2024

Uh oh!

Flarna commented Dec 23, 2024

Uh oh!

aduh95 commented Feb 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

Conversation

huseyinacacak-janea commented Oct 31, 2024

Uh oh!

avivkeller Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

huseyinacacak-janea Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

avivkeller Oct 31, 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 Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Nov 1, 2024

Uh oh!

aduh95 commented Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 5, 2024

Uh oh!

Flarna commented Dec 23, 2024

Uh oh!

aduh95 commented Feb 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

avivkeller Oct 31, 2024 •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading

aduh95 commented Nov 3, 2024 •

edited

Loading