-
Notifications
You must be signed in to change notification settings - Fork 206
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
exFAT workaround #5492
exFAT workaround #5492
Conversation
src/Gaffer/FileSystemPath.cpp
Outdated
) && e.value() == 87 // "The parameter is incorrect." | ||
) | ||
{ | ||
std::cerr << e.value() << " ( " << e.category().name() << " ) : " << e.message() << "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this might be left over from debugging?
src/Gaffer/FileSystemPath.cpp
Outdated
const std::filesystem::file_type t = std::filesystem::symlink_status( std::filesystem::path( this->string() ), e ).type(); | ||
std::filesystem::file_type t = std::filesystem::symlink_status( std::filesystem::path( this->string() ), e ).type(); | ||
|
||
#if _MSC_VER < 1932 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this evaluate to on Linux? Is there any chance _MSC_VER
ends up being interpreted as 0
because it's not defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know that macro values evaluated to 0 if undefined, good to know. Fixed in dd06cf5
e0663da
to
dd06cf5
Compare
I addressed those two comments in dd06cf5, but it sounds like we can't yet confirm it's solving the problem just yet. I'll continue troubleshooting on the chat thread (https://groups.google.com/g/gaffer-dev/c/zuAhqDFIl50) |
MSVC's `std::filesystem` has a bug that causes it to fail to retrieve file information on exFAT partitions in versions prior to (roughly) 17.2. That version is the first release after merging a fix from microsoft/STL#2373, but I'm not certain that version has the fix included. Using `std::filesystem::status` instead of `symlink_status()` seems justified because exFAT paritions don't support symlinks. If other partition types are included in the filter for error code 87, this may need to be revisited.
After talking it over today, I think this is ready for merging if we're happy the the commit itself. It seems to fix the exFAT problem without side effects and the user's crash sounds like it's probably unrelated to exFAT mischief. |
I rebased to fix the |
This works around a bug in MSVC's standard library causing
symlink_status()
to fail on exFAT (and maybe FAT) partitions. That bug was fixed in microsoft/STL#233, but this would be a useful fix prior to moving to MSVC 2022.It doesn't satisfy #5490, but does workaround one of the problems mentioned in that issue.
Checklist