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

module,win: fix long path resolve #53294

Merged

Conversation

huseyinacacak-janea
Copy link
Contributor

Several issues were encountered with module loading due to extended path lengths on Windows. These have been addressed with corrections implemented across four distinct code locations, accompanied by the addition of a corresponding test case for each to ensure functionality.
Additionally, I've moved es-module/test-GH-50753.js to es-module/test-esm-long-path-win.js as the tests cover more cases than specified in that issue.

Fixes: #50753

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 3, 2024
@targos
Copy link
Member

targos commented Jun 3, 2024

@nodejs/platform-windows

H4ad
H4ad previously approved these changes Jun 3, 2024
@H4ad H4ad dismissed their stale review June 3, 2024 12:53

I actually need more time to take a look, I think the implementation will solve the problem but I want to test in which cases we should include the UNC path, since we didn't do this in all paths in the toNamespacedPath.

@H4ad
Copy link
Member

H4ad commented Jun 3, 2024

There are some conditions to include the UNC that you are not following:

node/lib/path.js

Lines 632 to 645 in 58711c2

if (StringPrototypeCharCodeAt(resolvedPath, 0) === CHAR_BACKWARD_SLASH) {
// Possible UNC root
if (StringPrototypeCharCodeAt(resolvedPath, 1) === CHAR_BACKWARD_SLASH) {
const code = StringPrototypeCharCodeAt(resolvedPath, 2);
if (code !== CHAR_QUESTION_MARK && code !== CHAR_DOT) {
// Matched non-long UNC root, convert the path to a long UNC path
return `\\\\?\\UNC\\${StringPrototypeSlice(resolvedPath, 2)}`;
}
}
} else if (
isWindowsDeviceRoot(StringPrototypeCharCodeAt(resolvedPath, 0)) &&
StringPrototypeCharCodeAt(resolvedPath, 1) === CHAR_COLON &&
StringPrototypeCharCodeAt(resolvedPath, 2) === CHAR_BACKWARD_SLASH
) {

Although the condition could be matched, I think it would be better to wait for the migration of toNamespacedPath to C++.

Maybe we could include the ToNamespacePath port in this PR, and then leave that other PR just to remove the JS version and rewrite the places they are used.

@anonrig What do you think?

@StefanStojanovic StefanStojanovic added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2024
@nodejs-github-bot
Copy link
Collaborator

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

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

I prefer to land this after the toNamespacedPath migration to C++. Unfortunately, I don't have the time to pursue this at the moment (afaict, there is only one failing test). I appreciate if you could took over that PR and apply these changes in a follow up PR.

@MoLow
Copy link
Member

MoLow commented Jun 3, 2024

I prefer to land this after the toNamespacedPath migration to C++. Unfortunately, I don't have the time to pursue this at the moment (afaict, there is only one failing test). I appreciate if you could took over that PR and apply these changes in a follow up PR.

in that case, can you convert the blocking "request changes" into a simple suggestion/friendly ask? I don't think this PR should be blocked on a PR that is not being currently worked on

@nodejs-github-bot
Copy link
Collaborator

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

@huseyinacacak-janea
Copy link
Contributor Author

I prefer to land this after the toNamespacedPath migration to C++. Unfortunately, I don't have the time to pursue this at the moment (afaict, there is only one failing test). I appreciate if you could took over that PR and apply these changes in a follow up PR.

Sure, no problem, I'll take over your PR and start working on it.

@StefanStojanovic StefanStojanovic added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 2, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 3, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@huseyinacacak-janea
Copy link
Contributor Author

I prefer to land this after the toNamespacedPath migration to C++. Unfortunately, I don't have the time to pursue this at the moment (afaict, there is only one failing test). I appreciate if you could took over that PR and apply these changes in a follow up PR.

@anonrig I've rebased this PR to use toNamespacedPath. Could you please review it?

@huseyinacacak-janea
Copy link
Contributor Author

Is there anything else I can do to help this PR move forward?

@H4ad H4ad requested a review from anonrig July 15, 2024 12:01
src/node_file.cc Outdated
switch (FilePathIsFile(env, file_path)) {
Local<Value> local_file_path =
Buffer::Copy(
env->isolate(), file_path.c_str(), strlen(file_path.c_str()))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
env->isolate(), file_path.c_str(), strlen(file_path.c_str()))
env->isolate(), file_path.c_str(), file_path.size())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks.

src/node_file.cc Outdated
switch (FilePathIsFile(env, file_path)) {
Local<Value> local_file_path =
Buffer::Copy(
env->isolate(), file_path.c_str(), strlen(file_path.c_str()))
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

// Check if the path has a trailing slash. If so, add it after
// ToNamespacedPath() as it will be deleted by ToNamespacedPath()
bool slashCheck = !path_value.ToString().empty() &&
path_value.ToString().back() ==
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
path_value.ToString().back() ==
path_value.ToStringView().ends_with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

// Check if the path has a trailing slash. If so, add it after
// ToNamespacedPath() as it will be deleted by ToNamespacedPath()
bool slashCheck = !path_value.ToString().empty() &&
path_value.ToString().back() ==
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -323,8 +323,8 @@ void BindingData::GetNearestParentPackageJSON(
// Check if the path has a trailing slash. If so, add it after
// ToNamespacedPath() as it will be deleted by ToNamespacedPath()
bool slashCheck = !path_value.ToString().empty() &&
path_value.ToString().back() ==
std::filesystem::path::preferred_separator;
path_value.ToStringView().ends_with(
Copy link
Member

Choose a reason for hiding this comment

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

The previous line is not needed path_value.ToString() creates an unnecessary string.

Can you also remove this in other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@huseyinacacak-janea
Copy link
Contributor Author

I've fixed the suggestions. Is there anything else I can do to help this PR move forward?

@QuantumQuin
Copy link

@anonrig Is there anything else we can do to help this PR move forward? I'm having many issues with long paths on Windows and am eager to get this fixed.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@huseyinacacak-janea
Copy link
Contributor Author

The failing test in the CI is a known flaky test.

@nodejs-github-bot
Copy link
Collaborator

@H4ad H4ad added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 10, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 10, 2024
@nodejs-github-bot nodejs-github-bot merged commit 37f9eca into nodejs:main Aug 10, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 37f9eca

targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #53294
Fixes: #50753
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #53294
Fixes: #50753
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
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++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

Long node_modules paths cannot be found on Windows when LongPathsEnabled enabled
8 participants