Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "path,win: fix bug in resolve and normalize" #56088

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Nov 29, 2024

This reverts commit 7f68e54.

fixes: #56002

Additionally add a test to verify enumeration of windows named pipes works.


Seems normalization of path names of devices requires the trailing \ at least in some cases like debugger.

Not sure if revert is the best way to go, maybe there is a better fix. Happy to close this revert PR in case of a better proposal.

fyi @huseyinacacak-janea

Add a test to verify named pipe enumeration in windows works.
@nodejs-github-bot 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 Nov 29, 2024
Copy link

codecov bot commented Nov 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.99%. Comparing base (4ee87b8) to head (ccd6228).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56088      +/-   ##
==========================================
- Coverage   88.00%   87.99%   -0.01%     
==========================================
  Files         656      656              
  Lines      189000   188989      -11     
  Branches    35995    35983      -12     
==========================================
- Hits       166320   166301      -19     
- Misses      15840    15853      +13     
+ Partials     6840     6835       -5     
Files with missing lines Coverage Δ
lib/path.js 95.19% <100.00%> (-0.04%) ⬇️
src/path.cc 69.04% <ø> (ø)

... and 27 files with indirect coverage changes

@ShenHongFei
Copy link
Contributor

After testing locally, I can use vscode to debug normally after merging.

@lpinca
Copy link
Member

lpinca commented Nov 30, 2024

If/when this lands, #54025 should be reopened.

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++. needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node 23.2 fs lost the ability to enumerate named pipes on Windows
4 participants