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

Allow longpath executables #22532

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Demiguise
Copy link

We ran into #19710 when we started to tinker with the Python rules and bazel, as we were running into extremely long execution times when running anything related to Python (45-60s). Disabling Python zipping and enabling runfiles on windows brought the execution times down and made them comparable to MacOS/Linux, which was a huge win.

Only issue is that the runfiles then ran straight into the wonderful longpath problems from the linked issue when running tests that now used runfiles. If 8dot3 names are not turned on, or there is no short path associated with the given path, then the AsShortPath function will fail. Even though "Technically" long paths could be enabled and windows could actually still execute the requested executable.

Although CreateProcessW has the MAX_PATH limit built into it, which can never be worked around, batch files seemingly do not have this issue. So, we can simply dump the requested command line into a batch file and redirect the command line to run that batch file instead.

This is a similar approach to CMake/Ninja when a CustomCommand is over a certain (8192) character limit. In that situation they create a new batch file that has the command inside and ensure that the Ninja target just runs that file instead.

@Demiguise Demiguise force-pushed the allow_longpath_executables branch from 25bc69a to 5879ac7 Compare May 24, 2024 11:45
@Demiguise Demiguise marked this pull request as ready for review December 11, 2024 13:58
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Dec 11, 2024
@meisterT meisterT added the area-Windows Windows-specific issues and feature requests label Dec 12, 2024
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

All looks reasonable to me, thanks for the contribution!

Can you please add a test in https://cs.opensource.google/bazel/bazel/+/master:src/test/py/bazel/bazel_windows_test.py?

stem.remove_suffix(1);
}

std::wstring wrapper_file_name = std::wstring(stem) + L".wrapper.bat";
Copy link
Member

Choose a reason for hiding this comment

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

This might be risk of conflict with existing file or lack of write permission? Can we find a safer place to create the wrapper?

@meteorcloudy meteorcloudy added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Dec 13, 2024
@Demiguise
Copy link
Author

All looks reasonable to me, thanks for the contribution!

Can you please add a test in https://cs.opensource.google/bazel/bazel/+/master:src/test/py/bazel/bazel_windows_test.py?

Sure, will also try and fix the string view compilation failures ;.;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Windows Windows-specific issues and feature requests awaiting-review PR is awaiting review from an assigned reviewer team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants