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

fs: fix fs.promises.realpath for long paths on Windows #51032

Conversation

sapphi-red
Copy link
Contributor

This PR is almost the same with #44536 but for fs.promises.realpath.

Fixes: #51031

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Dec 3, 2023
@sapphi-red sapphi-red force-pushed the fix-win-fs-promises-realpath-long-paths branch from 6b4a8d5 to 7d4a207 Compare December 3, 2023 12:23
@sapphi-red sapphi-red marked this pull request as ready for review December 3, 2023 12:26
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

The code LGTM.

Nit: although win is a valid subsystem, it fits, and #44536 used it; IMHO fs: fix fs.promises.realpath for long paths on Windows might be a better commit message.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Unlike other fs functions that work with paths, realpath isn't
using pathModule.toNamespacedPath prior to calling libuv function. This
is causing issues on windows.

Windows long path test is also improved to cover the mentioned issue.

Fixes: nodejs#51031
@sapphi-red sapphi-red force-pushed the fix-win-fs-promises-realpath-long-paths branch from 7d4a207 to cd07613 Compare December 5, 2023 14:08
@sapphi-red sapphi-red changed the title win: fix fs.promises.realpath for long paths fs: fix fs.promises.realpath for long paths on Windows Dec 5, 2023
@sapphi-red
Copy link
Contributor Author

I've updated the commit message 👍 (The force push includes no code changes)

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 28, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 28, 2023
@nodejs-github-bot nodejs-github-bot merged commit 4944e97 into nodejs:main Dec 28, 2023
53 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4944e97

@sapphi-red sapphi-red deleted the fix-win-fs-promises-realpath-long-paths branch December 30, 2023 08:05
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
Unlike other fs functions that work with paths, realpath isn't
using pathModule.toNamespacedPath prior to calling libuv function. This
is causing issues on windows.

Windows long path test is also improved to cover the mentioned issue.

Fixes: #51031
PR-URL: #51032
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Unlike other fs functions that work with paths, realpath isn't
using pathModule.toNamespacedPath prior to calling libuv function. This
is causing issues on windows.

Windows long path test is also improved to cover the mentioned issue.

Fixes: #51031
PR-URL: #51032
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Unlike other fs functions that work with paths, realpath isn't
using pathModule.toNamespacedPath prior to calling libuv function. This
is causing issues on windows.

Windows long path test is also improved to cover the mentioned issue.

Fixes: #51031
PR-URL: #51032
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fs.promises.realpath errors with long paths (fs.realpath.native does not error)
8 participants