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

Closed
wants to merge 2 commits into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Nov 29, 2024

This reverts commit 7f68e54.

fixes: #56002
Refs: #55623
Reopens: #54025

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

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Nov 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2024
@nodejs-github-bot
Copy link
Collaborator

@huseyinacacak-janea
Copy link
Contributor

huseyinacacak-janea commented Dec 2, 2024

@Flarna thank you for tagging me. I'll open a PR to fix #56002 today or tomorrow. If the new PR lands, this revert will not be necessary.

@Flarna
Copy link
Member Author

Flarna commented Dec 2, 2024

@huseyinacacak-janea fine for me. Please include the test I added here (second commit) or a similar one in your PR.

@Flarna Flarna added the revert PRs that revert previously landed PRs. label Dec 2, 2024
@Flarna
Copy link
Member Author

Flarna commented Dec 5, 2024

closing in favor of #56110

@Flarna Flarna closed this Dec 5, 2024
@Flarna Flarna deleted the revert-win-path-resolve branch December 5, 2024 17:38
nodejs-github-bot pushed a commit that referenced this pull request Dec 10, 2024
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
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
PR-URL: #56110
Fixes: #56002
Refs: #55623
Refs: #56088
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. revert PRs that revert previously landed PRs.
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
7 participants