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

build: add long paths awareness on windows #44522

Closed
wants to merge 1 commit into from

Conversation

StefanStojanovic
Copy link
Contributor

On windows when using fs.realpath.native, fs.realpathSync.native and fs.promises.realpath on long path, ENOENT is thrown. On the other hand, long paths work well with fs.realpath and fs.realpathSync. On Linux, all of the mentioned realpath versions work as expected.

The problem with native realpath implementation on windows was, that by default applications aren't long path aware, unless specified otherwise by their manifest file, which was the case with node. The other way to enable working with long paths is to add a \\?\ prefix to path prior to using it in windows API functions. This approach is used by all fs functions working with paths (they add prefix by calling pathModule.toNamespacedPath(path)), but this is not done in fs.realpath.native and fs.realpathSync.native.

Although fix could have been made in fs.js, a change to node manifest file was chosen as a more comprehensive and broader solution to the long paths on windows problem in general.

An existing windows long path test is improved to cover the issue that is fixed by these changes.

Fixes: #39721

Libuv has a manifest file to enable working with long paths. Node does
not have that in its manifest and that leads to problems in some cases
on Windows (eg. realpath.native).

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

Fixes: nodejs#39721
@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. labels Sep 5, 2022
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2022
@nodejs-github-bot
Copy link
Collaborator

@Flarna
Copy link
Member

Flarna commented Sep 6, 2022

@StefanStojanovic It seems the test failure is related to your change. Please take a look.

@StefanStojanovic
Copy link
Contributor Author

@StefanStojanovic It seems the test failure is related to your change. Please take a look.

Yes, tests failed because of my change. The problem with it is that manifest file long path awareness relies on registry key HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabled existing and being set to 1 plus it only works on Windows 10 version 1607 and later. So, since this solution isn't guaranteed to work in all cases, I made another PR focusing the fs.reaplath.native issue specifically: #44536

@lpinca
Copy link
Member

lpinca commented Sep 6, 2022

Does it make sense to land both this and #44536?

@lpinca
Copy link
Member

lpinca commented Sep 6, 2022

Does it make sense to land both this and #44536?

I wonder if it is better to use a single commit to facilitate backporting without breaking tests.

@StefanStojanovic
Copy link
Contributor Author

Does it make sense to land both this and #44536?

My idea was to just land #44536. Since manifest file fix isn't guaranteed to work, I'm thinking this PR should probably be closed, just wanted to keep it open for reference until the other one lands.

@lpinca
Copy link
Member

lpinca commented Sep 6, 2022

Ok.

@lpinca
Copy link
Member

lpinca commented Sep 6, 2022

Superseded by #44536.

@Flarna
Copy link
Member

Flarna commented Sep 13, 2022

@StefanStojanovic I guess we can close this one as #44536 landed?

@StefanStojanovic
Copy link
Contributor Author

@StefanStojanovic I guess we can close this one as #44536 landed?

Yes

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.
Projects
None yet
5 participants