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

ARROW-8477: [C++] Enable reading and writing of long filenames for Windows #6993

Closed
wants to merge 7 commits into from
Closed

Conversation

tpboudreau
Copy link
Contributor

This patch enables reading/writing of files with long (>260 characters) pathnames in Windows.

In order for the new test to run under Windows, both (1) the test host must have long paths enabled in its registry, and (2) the test executable (arrow_utility_test.exe) must include a manifest indicating support for long paths (see https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#enable-long-paths-in-windows-10-version-1607-and-later). The test source code checks for (1) and the cmake file changes ensure (2).

@github-actions
Copy link

@@ -446,6 +446,56 @@ TEST(CreateDirTree, Basics) {
ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("EF"));
ASSERT_OK_AND_ASSIGN(created, CreateDirTree(fn));
ASSERT_TRUE(created);

#ifndef __APPLE__
Copy link
Member

Choose a reason for hiding this comment

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

Why did you have to disable this test on macOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I experienced failures in the CI pipeline on macOS and I was unable to locate clear documentation of the path name limits (I'm not a macOS expert).

I figured it might be best to separately address macOS in another issue, if there's community support for that. (This patch leaves macOS builds and tests unchanged.)

If you believe this test should run as is under macOS, I'll remove the #ifndef and follow up on any issues.

Copy link
Member

Choose a reason for hiding this comment

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

I was just wondering. According to Google searches, the path length limit on macOS may be 1024, which this test exceeds. We can keep it like that.

@pitrou
Copy link
Member

pitrou commented Apr 20, 2020

Looks like Windows long paths are enabled by default on Github Actions. Cool!

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks for the report and for the fix. WIll merge if CI green.

@tpboudreau
Copy link
Contributor Author

tpboudreau commented Apr 20, 2020

Your fixups look good. Thanks!

@pitrou
Copy link
Member

pitrou commented Apr 20, 2020

The remaining CI failure is unrelated.

@pitrou pitrou closed this in 1b71ca7 Apr 20, 2020
kszucs pushed a commit that referenced this pull request Apr 20, 2020
…ndows

This patch enables reading/writing of files with long (>260 characters) pathnames in Windows.

In order for the new test to run under Windows, both (1) the test host must have long paths enabled in its registry, and (2) the test executable (arrow_utility_test.exe) must include a manifest indicating support for long paths (see https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#enable-long-paths-in-windows-10-version-1607-and-later).  The test source code checks for (1) and the cmake file changes ensure (2).

Closes #6993 from tpboudreau/ARROW-8477

Lead-authored-by: TP Boudreau <tpboudreau@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@tpboudreau
Copy link
Contributor Author

Thanks @pitrou for jumping on this so quickly.

@tpboudreau tpboudreau deleted the ARROW-8477 branch April 23, 2020 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants