GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: revert filesystem::path changes by anonrig · Pull Request #55015 · nodejs/node

Conversation

@anonrig

@anonrig anonrig commented Sep 19, 2024

Copy link
Copy Markdown
Member

Reverts #53063. There are still some filesystem::path references left, since they're added in multiple different PRs. Depending on whether we need them or not, we can remove them.

We're reverting because it fixes multiple different regressions due to Windows UTF-16 path requirements.

Fixes #54991
Fixes #54476

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and LightSpeedC reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@anonrig
anonrig requested review from joyeecheung and richardlau September 19, 2024 17:12
@nodejs-github-bot

nodejs-github-bot commented Sep 19, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/security-wg
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Sep 19, 2024
@anonrig
anonrig force-pushed the revert-filesystem-path branch 3 times, most recently from eea22f7 to 37bd2dc Compare September 19, 2024 17:17
benjamingr
benjamingr approved these changes Sep 19, 2024
View reviewed changes
@anonrig
anonrig force-pushed the revert-filesystem-path branch 2 times, most recently from be2b0b8 to b0e83d9 Compare September 19, 2024 17:21
@anonrig

anonrig commented Sep 19, 2024

Copy link
Copy Markdown
Member Author

@joyeecheung Can you double check the changes on compile_cache.cc and header file? I manually had to change those lines, there was a conflict.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig commented Sep 19, 2024
View reviewed changes
Comment thread src/permission/fs_permission.cc Outdated

@anonrig anonrig Sep 19, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@joyeecheung Is it safe to keep this function, or do we need this revert?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Sep 20, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is std::filesystem::path(res) correct? From what I can tell, res contains UTF8-encoded data, and it's about to be passed into uv_fs_stat which also expects UTF8-encoded data. So std::filesystem::path(res) here is also going to crash on Windows when res contains code points that are encoded differently in UTF16 unless it's casted between u8string both ways.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@anonrig
anonrig force-pushed the revert-filesystem-path branch from b0e83d9 to 67ddaa9 Compare September 19, 2024 17:23
@avivkeller avivkeller added fs Issues and PRs related to file-system APIs and the fs module. revert PRs that revert previously landed PRs. labels Sep 19, 2024
@avivkeller

avivkeller commented Sep 19, 2024

Copy link
Copy Markdown
Member

Is it possible to add a test for those issues?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
src: revert filesystem::path changes
ba26f0b
@anonrig
anonrig force-pushed the revert-filesystem-path branch from 67ddaa9 to ba26f0b Compare September 19, 2024 17:36
@codecov

codecov Bot commented Sep 19, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.04%. Comparing base (291d90a) to head (ba26f0b).
Report is 461 commits behind head on main.

Files with missing lines Patch % Lines
src/env.cc 50.00% 1 Missing ⚠️
src/node_modules.cc 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #55015   +/-   ##
=======================================
  Coverage   88.04%   88.04%           
=======================================
  Files         652      652           
  Lines      183764   183760    -4     
  Branches    35862    35863    +1     
=======================================
  Hits       161787   161787           
+ Misses      15233    15230    -3     
+ Partials     6744     6743    -1
Files with missing lines Coverage Δ
src/compile_cache.cc 79.59% <100.00%> (-0.41%) ⬇️
src/compile_cache.h 100.00% <100.00%> (ø)
src/inspector_profiler.cc 78.79% <100.00%> (+0.26%) ⬆️
src/node_file.cc 77.03% <100.00%> (-0.02%) ⬇️
src/node_report.cc 93.15% <100.00%> (ø)
src/path.cc 67.39% <100.00%> (ø)
src/permission/fs_permission.cc 73.22% <100.00%> (+1.68%) ⬆️
src/permission/fs_permission.h 91.80% <100.00%> (ø)
src/util.h 89.91% <ø> (ø)
src/env.cc 85.58% <50.00%> (-0.02%) ⬇️
... and 1 more

... and 29 files with indirect coverage changes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Sep 19, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62569/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Sep 20, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62582/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung approved these changes Sep 20, 2024
View reviewed changes

@joyeecheung joyeecheung left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM % nits

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compile_cache.cc
std::u8string cache_dir_with_tag_u8 = cache_dir_with_tag.u8string();
std::string cache_dir_with_tag_str(cache_dir_with_tag_u8.begin(),
cache_dir_with_tag_u8.end());
std::string cache_dir_with_tag =

@joyeecheung joyeecheung Sep 20, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You can minimize the diff/potential conflicts if this is named as cache_dir_with_tag_str

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@joyeecheung

joyeecheung commented Sep 20, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

By the way I don't think this fixes #54476 because fs.cpSync changes were done in #53614 and #54653 is fixing that with the casting, as the path is going to be consumed by std::filesystem::create_directories at the end.
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 21, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 21, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 5a96671 into nodejs:main Sep 21, 2024
@nodejs-github-bot

nodejs-github-bot commented Sep 21, 2024

Copy link
Copy Markdown
Collaborator

Landed in 5a96671
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request Oct 4, 2024
@anonrig @targos
src: revert filesystem::path changes
9ae80e1 
PR-URL: #55015
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
V22.10.0 proposal #55343
Merged
@marco-ippolito marco-ippolito added the dont-land-on-v20.x label Nov 16, 2024
@joyeecheung joyeecheung mentioned this pull request Nov 29, 2024
fs.rmSync('速') crash without throw #56049
Closed
codebytere added a commit to electron/electron that referenced this pull request Jan 22, 2025
@codebytere
src: revert filesystem::path changes
c7552ad 
nodejs/node#55015
samuelmaddock pushed a commit to electron/electron that referenced this pull request Jan 22, 2025
@electron-roller @codebytere
chore: bump node to v22.11.0 (main) (#44530)
863faea 
* chore: bump node in DEPS to v22.11.0

* src: move evp stuff to ncrypto

nodejs/node#54911

* crypto: add Date fields for validTo and validFrom

nodejs/node#54159

* module: fix discrepancy between .ts and .js

nodejs/node#54461

* esm: do not interpret "main" as a URL

nodejs/node#55003

* src: modernize likely/unlikely hints

nodejs/node#55155

* chore: update patch indices

* crypto: add validFromDate and validToDate fields to X509Certificate

nodejs/node#54159

* chore: fixup perfetto patch

* fix: clang warning in simdjson

* src: add receiver to fast api callback methods

nodejs/node#54408

* chore: fixup revert patch

* fixup! esm: do not interpret "main" as a URL

* fixup! crypto: add Date fields for validTo and validFrom

* fix: move ArrayBuffer test patch

* src: fixup Error.stackTraceLimit during snapshot building

nodejs/node#55121

* fix: bad rebase

* chore: fixup amaro

* chore: address feedback from review

* src: revert filesystem::path changes

nodejs/node#55015

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Feb 3, 2025
@codebytere
src: revert filesystem::path changes
eb130b0 
nodejs/node#55015
codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
@codebytere
src: revert filesystem::path changes
c178633 
nodejs/node#55015
codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
@codebytere
src: revert filesystem::path changes
2aceefd 
nodejs/node#55015
codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025
@codebytere
src: revert filesystem::path changes
9c2550b 
nodejs/node#55015
codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025
@electron-roller @codebytere
chore: bump node to v22.13.1 (35-x-y) (#45216)
bc163a5 
* chore: bump node in DEPS to v22.13.0

* chore: bump node in DEPS to v22.13.1

* src: move evp stuff to ncrypto

nodejs/node#54911

* crypto: add Date fields for validTo and validFrom

nodejs/node#54159

* module: fix discrepancy between .ts and .js

nodejs/node#54461

* esm: do not interpret "main" as a URL

nodejs/node#55003

* src: modernize likely/unlikely hints

nodejs/node#55155

* chore: update patch indices

* crypto: add validFromDate and validToDate fields to X509Certificate

nodejs/node#54159

* chore: fixup perfetto patch

* fix: clang warning in simdjson

* src: add receiver to fast api callback methods

nodejs/node#54408

* chore: fixup revert patch

* fixup! esm: do not interpret "main" as a URL

* fixup! crypto: add Date fields for validTo and validFrom

* fix: move ArrayBuffer test patch

* src: fixup Error.stackTraceLimit during snapshot building

nodejs/node#55121

* fix: bad rebase

* chore: fixup amaro

* chore: address feedback from review

* src: revert filesystem::path changes

nodejs/node#55015

* chore: fixup GN build file

* nodejs/node#55529
* nodejs/node#55798
* nodejs/node#55530

* module: simplify --inspect-brk handling

nodejs/node#55679

* src: fix outdated js2c.cc references

nodejs/node#56133

* crypto: include openssl/rand.h explicitly

nodejs/node#55425

* build: use variable for crypto dep path

nodejs/node#55928

* crypto: fix RSA_PKCS1_PADDING error message

nodejs/node#55629

* build: use variable for simdutf path

nodejs/node#56196

* test,crypto: make crypto tests work with BoringSSL

nodejs/node#55491

* fix: suppress clang -Wdeprecated-declarations in libuv

libuv/libuv#4486

* deps: update libuv to 1.49.1

nodejs/node#55114

* test: make test-node-output-v8-warning more flexible

nodejs/node#55401

* [v22.x] Revert "v8: enable maglev on supported architectures"

nodejs/node#54384

* fix: potential WIN32_LEAN_AND_MEAN redefinition

c-ares/c-ares#869

* deps: update nghttp2 to 1.64.0

nodejs/node#55559

* src: provide workaround for container-overflow

nodejs/node#55591

* build: use variable for simdutf path

nodejs/node#56196

* chore: fixup patch indices

* fixup! module: simplify --inspect-brk handling

* lib: fix fs.readdir recursive async

nodejs/node#56041

* lib: avoid excluding symlinks in recursive fs.readdir with filetypes

nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up

* test: disable CJS permission test for config.main

This has diverged as a result of our revert of
src,lb: reducing C++ calls of esm legacy main resolve

* fixup! lib: fix fs.readdir recursive async

* deps: update libuv to 1.49.1

nodejs/node#55114

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
@YEONDG YEONDG mentioned this pull request Jul 25, 2025
Node.js fails to run when folder path contains Korean characters #59208
Closed
@yinsang0910-star yinsang0910-star mentioned this pull request Oct 19, 2026
fix(desktop): mirror the bundled Connector source without the native fs.cpSync copy anywhere-labs/Agents-Anywhere#345
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes
@benjamingr benjamingr benjamingr approved these changes
@richardlau richardlau Awaiting requested review from richardlau

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to file-system APIs and the fs module. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. revert PRs that revert previously landed PRs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

v22.3.0 Cannot start if the Folder or Script File Name is Unicode. v22.6.0 fs.cpSync crash

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@anonrig @nodejs-github-bot @avivkeller @joyeecheung @benjamingr @marco-ippolito
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.