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.

Sign up for GitHub

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

Support for Windows Long Paths #398

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Support for Windows Long Paths #398

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c30a554
Support long paths on windows
Akarale98 Oct 14, 2024
1d57d3e
Pre commit fixes
Akarale98 Oct 14, 2024
3d196b2
Merge remote-tracking branch 'upstream/main' into windows-long-paths
Akarale98 Oct 30, 2024
8ff3bca
Addressing review comments
Akarale98 Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 37 additions & 4 deletions src/filesystem/implementations/local.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,45 @@ class LocalFileSystem : public FileSystem {
Status MakeTemporaryDirectory(
std::string dir_path, std::string* temp_dir) override;
Status DeletePath(const std::string& path) override;

private:
inline std::string getOSValidPath(const std::string& _path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
inline std::string getOSValidPath(const std::string& _path);
inline std::string GetOSValidPath(const std::string& path);

To match triton syntax conventions

const char* kWindowsLongPathPrefix = "\\\\?\\";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const char* kWindowsLongPathPrefix = "\\\\?\\";
const char* windows_long_path_prefix_ = "\\\\?\\";

To match Triton member variable convention.

Copy link

Choose a reason for hiding this comment

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

But that's a constant, not a variable? AFAIK, constants should be named like the former, not the latter.

Copy link

Choose a reason for hiding this comment

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

It might be more idiomatic to use static constexpr char* kWindowsLongPathPrefix = "\\\\?\\"; to drive the fact this is, indeed, a constant.

Copy link
Contributor

@fpetrini15 fpetrini15 Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Had to verify with the google style guide and you're both correct. I also found an instance in Triton code where we initialize to a constant here, however, we omit the leading "k", which is technically wrong according to the style standards.

We can defer to Nico's suggestion instead.

};


//! Converts incoming utf-8 path to an OS valid path
//!
//! On Linux there is not much to do but make sure correct slashes are used
//! On Windows we need to take care of the long paths and handle them correctly
//! to avoid legacy issues with MAX_PATH
//!
//! More details:
//! https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry
//!
inline std::string
LocalFileSystem::getOSValidPath(const std::string& _path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LocalFileSystem::getOSValidPath(const std::string& _path)
LocalFileSystem::getOSValidPath(const std::string& path)

Cascading effect where the local path variable will need to change to something like local_path or l_path

{
std::string path(_path);
#ifdef _WIN32
// On Windows long paths must be marked correctly otherwise, due to backwards
// compatibility, all paths are limited to MAX_PATH length
if (path.size() >= MAX_PATH) {
// Must be prefixed with "\\?\" to be considered long path
if (path.substr(0, 4) != (kWindowsLongPathPrefix)) {
// Long path but not "tagged" correctly
path = (kWindowsLongPathPrefix) + path;
}
}
std::replace(path.begin(), path.end(), '/', '\\');
#endif
return path;
}

Status
LocalFileSystem::FileExists(const std::string& path, bool* exists)
{
*exists = (access(path.c_str(), F_OK) == 0);
*exists = (access(getOSValidPath(path).c_str(), F_OK) == 0);
return Status::Success;
}

Expand All @@ -75,7 +108,7 @@ LocalFileSystem::IsDirectory(const std::string& path, bool* is_dir)
*is_dir = false;

struct stat st;
if (stat(path.c_str(), &st) != 0) {
if (stat(getOSValidPath(path).c_str(), &st) != 0) {
return Status(Status::Code::INTERNAL, "failed to stat file " + path);
}

Expand All @@ -88,7 +121,7 @@ LocalFileSystem::FileModificationTime(
const std::string& path, int64_t* mtime_ns)
{
struct stat st;
if (stat(path.c_str(), &st) != 0) {
if (stat(getOSValidPath(path).c_str(), &st) != 0) {
return Status(Status::Code::INTERNAL, "failed to stat file " + path);
}

Expand Down Expand Up @@ -187,7 +220,7 @@ LocalFileSystem::GetDirectoryFiles(
Status
LocalFileSystem::ReadTextFile(const std::string& path, std::string* contents)
{
std::ifstream in(path, std::ios::in | std::ios::binary);
std::ifstream in(getOSValidPath(path), std::ios::in | std::ios::binary);
if (!in) {
return Status(
Status::Code::INTERNAL,
Expand Down