src: revert filesystem::path changes #55015

anonrig · 2024-09-19T17:12:38Z

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

nodejs-github-bot · 2024-09-19T17:12:44Z

Review requested:

@nodejs/security-wg

anonrig · 2024-09-19T17:21:46Z

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

anonrig · 2024-09-19T17:22:44Z

src/permission/fs_permission.cc

- auto file_status = std::filesystem::status(path);
- if (file_status.type() == std::filesystem::file_type::directory) {
- path /= "*";
+ uv_fs_t req;


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

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.

avivkeller · 2024-09-19T17:31:06Z

Is it possible to add a test for those issues?

codecov · 2024-09-19T19:03:12Z

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

nodejs-github-bot · 2024-09-19T19:07:01Z

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

nodejs-github-bot · 2024-09-20T01:34:58Z

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

joyeecheung

LGTM % nits

joyeecheung · 2024-09-20T09:01:05Z

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 =


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

joyeecheung · 2024-09-20T09:09:07Z

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.

nodejs-github-bot · 2024-09-21T17:22:23Z

Landed in 5a96671

PR-URL: #55015 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs#55015 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs/node#55015

* 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 <shelley.vohr@gmail.com>

nodejs/node#55015

* 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 <shelley.vohr@gmail.com>

anonrig requested review from joyeecheung and richardlau September 19, 2024 17:12

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 19, 2024

anonrig force-pushed the revert-filesystem-path branch 3 times, most recently from eea22f7 to 37bd2dc Compare September 19, 2024 17:17

benjamingr approved these changes Sep 19, 2024

View reviewed changes

anonrig force-pushed the revert-filesystem-path branch 2 times, most recently from be2b0b8 to b0e83d9 Compare September 19, 2024 17:21

anonrig commented Sep 19, 2024

View reviewed changes

anonrig force-pushed the revert-filesystem-path branch from b0e83d9 to 67ddaa9 Compare September 19, 2024 17:23

avivkeller added fs Issues and PRs related to the fs subsystem / file system. revert PRs that revert previously landed PRs. labels Sep 19, 2024

src: revert filesystem::path changes

ba26f0b

anonrig force-pushed the revert-filesystem-path branch from 67ddaa9 to ba26f0b Compare September 19, 2024 17:36

joyeecheung approved these changes Sep 20, 2024

View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 21, 2024

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

nodejs-github-bot merged commit 5a96671 into nodejs:main Sep 21, 2024

targos pushed a commit that referenced this pull request Oct 4, 2024

src: revert filesystem::path changes

9ae80e1

PR-URL: #55015 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

aduh95 mentioned this pull request Oct 9, 2024

V22.10.0 proposal #55343

Merged

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024

src: revert filesystem::path changes

733eec9

PR-URL: nodejs#55015 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 16, 2024

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024

src: revert filesystem::path changes

0200cd7

PR-URL: nodejs#55015 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

joyeecheung mentioned this pull request Nov 29, 2024

fs.rmSync('速') crash without throw #56049

Open

codebytere added a commit to electron/electron that referenced this pull request Jan 22, 2025

src: revert filesystem::path changes

c7552ad

nodejs/node#55015

codebytere added a commit to electron/electron that referenced this pull request Feb 3, 2025

src: revert filesystem::path changes

eb130b0

nodejs/node#55015

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025

src: revert filesystem::path changes

c178633

nodejs/node#55015

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025

src: revert filesystem::path changes

2aceefd

nodejs/node#55015

codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025

src: revert filesystem::path changes

9c2550b

nodejs/node#55015

YEONDG mentioned this pull request Jul 25, 2025

Node.js fails to run when folder path contains Korean characters #59208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

src: revert filesystem::path changes #55015

src: revert filesystem::path changes #55015

Uh oh!

anonrig commented Sep 19, 2024

nodejs-github-bot commented Sep 19, 2024

anonrig commented Sep 19, 2024

anonrig Sep 19, 2024

joyeecheung Sep 20, 2024 •

edited

Loading

avivkeller commented Sep 19, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

nodejs-github-bot commented Sep 19, 2024

nodejs-github-bot commented Sep 20, 2024

joyeecheung left a comment

joyeecheung Sep 20, 2024

joyeecheung commented Sep 20, 2024 •

edited

Loading

nodejs-github-bot commented Sep 21, 2024

Labels

6 participants

Uh oh!

src: revert filesystem::path changes #55015

src: revert filesystem::path changes #55015

Uh oh!

Conversation

anonrig commented Sep 19, 2024

nodejs-github-bot commented Sep 19, 2024

anonrig commented Sep 19, 2024

anonrig Sep 19, 2024

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

avivkeller commented Sep 19, 2024

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

Uh oh!

Codecov Report

nodejs-github-bot commented Sep 19, 2024

nodejs-github-bot commented Sep 20, 2024

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung Sep 20, 2024

Choose a reason for hiding this comment

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

Uh oh!

nodejs-github-bot commented Sep 21, 2024

Labels

6 participants

joyeecheung Sep 20, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

joyeecheung commented Sep 20, 2024 •

edited

Loading