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

Adjusted behavior of filesystem::is_empty and filesystem::directory_iterator to work with empty volumes #4311

Conversation

MartinKuschnik
Copy link
Contributor

@MartinKuschnik MartinKuschnik commented Jan 12, 2024

The goal of the implementation was to fix #4291.

Following tables should explain the behavior of the current implementation in comparison to the changed code:

filesystem::is_empty(...)

Case filesystem::exists(...) current implementation this implementation
empty volume
\\?\Volume{ecab883f-c728-474b-9b40-e90cb2834b66}\
true ERROR 🐛 true
not existing volume
\\?\Volume{ecab883f-c728-474b-9b40-e90cb2834b65}\
false ERROR ERROR

filesystem::directory_iterator(...)

Case filesystem::exists(...) current implementation this implementation
empty volume
\\?\Volume{ecab883f-c728-474b-9b40-e90cb2834b66}\
true ERROR 🐛 empty
not existing volume
\\?\Volume{ecab883f-c728-474b-9b40-e90cb2834b65}\
false ERROR ERROR

@MartinKuschnik MartinKuschnik requested a review from a team as a code owner January 12, 2024 17:38
@StephanTLavavej StephanTLavavej added bug Something isn't working filesystem C++17 filesystem labels Jan 12, 2024
@StephanTLavavej

This comment was marked as resolved.

@MartinKuschnik
Copy link
Contributor Author

Hi Stephan,

thanks for your advice. I'm not used to this tool chain and struggling a bit with it.

After rechecking the readme, now I'm able to test in the right way instead of using the Test Explorer in Visual Studio. 😁

But I have two more questions.

  1. How can I check the format on my local machine?
  2. I'm getting the error 'Unexpected compiler version, expected MSVC 19.39 or newer.' with Visual Studio 17.9.0 Preview 2.1 The readme says Visual Studio 2022 17.9 Preview 2 or later. Do You have any idea what’s wrong with my setup?

@StephanTLavavej
Copy link
Member

How can I check the format on my local machine?

If you select the "C++ Clang tools for Windows" component in the VS Installer, that will install the proper version of clang-format. By default it'll be installed to C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/x64/bin/clang-format.exe. You can then use:

  • Microsoft's C/C++ Extension in VSCode, configured with "C_Cpp.clang_format_path": "C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/x64/bin/clang-format.exe"
  • Xaver Hellauer's Clang-Format extension in VSCode, configured with "clang-format.executable": "C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/x64/bin/clang-format.exe" (this is what I use)
  • Your favorite editor configured accordingly

In VSCode, if you open up our repo as a workspace, we automatically configure format-on-save via .vscode/settings.json so you just need an extension pointing at the right clang-format executable to make everything work.

If you have clang-format installed but don't want to format via your editor, you can ask our build system to format the entire repo with cmake --preset x64 && cmake --build --preset x64 --target format. This is less preferred because you have to remember to do it before every push. (The only time I use this command is when updating our clang-format version itself, or fixing up a branch that nobody has formatted at all.)

I'm getting the error 'Unexpected compiler version, expected MSVC 19.39 or newer.' with Visual Studio 17.9.0 Preview 2.1 The readme says Visual Studio 2022 17.9 Preview 2 or later. Do You have any idea what’s wrong with my setup?

It seems likely that you have a production version of VS installed (17.8?). That's fine (production happily coexists with Preview) but if your Command Prompt has 17.8 on its PATH then building microsoft/STL won't work with that. cl.exe will print its version, which is what we're inspecting.

You need to open up a VS Preview command prompt before working with the STL. What I do is start with a clean Command Prompt and then manually run "C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Auxiliary\Build\vcvarsall.bat" x64 to get the right version selected.

stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks for the fix - I wouldn't have known how to do this myself! 😻

I pushed a commit to address my code review comments, and I double-checked that the fix still works. After running your New-VHD incantation, I captured my volume name:

C:\Temp>type meow.cpp
#include <exception>
#include <filesystem>
#include <print>
using namespace std;

int main() {
    println("_MSVC_STL_UPDATE: {}", _MSVC_STL_UPDATE);

    constexpr const char* volume = R"(\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\)";

    println("filesystem::exists('{}')) : {}", volume, filesystem::exists(volume));

    try {
        println("filesystem::is_empty('{}')) succeeded: {}", volume, filesystem::is_empty(volume));
    } catch (const exception& ex) {
        println("filesystem::is_empty('{}')) failed: {}", volume, ex.what());
    }

    try {
        const filesystem::directory_iterator it(volume);
        println("filesystem::directory_iterator('{}')) succeeded.", volume);
    } catch (const exception& ex) {
        println("filesystem::directory_iterator('{}')) failed: {}", volume, ex.what());
    }
}

Then reproed the bug with VS 2022 17.9 Preview 2.1:

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /MTd /Od meow.cpp && meow
meow.cpp
_MSVC_STL_UPDATE: 202310
filesystem::exists('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) : true
filesystem::is_empty('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) failed: is_empty: The system cannot find the file specified.: "\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\"
filesystem::directory_iterator('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) failed: directory_iterator::directory_iterator: The system cannot find the file specified.: "\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\"

And verified the fix:

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /MTd /Od meow.cpp && meow
meow.cpp
_MSVC_STL_UPDATE: 202401
filesystem::exists('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) : true
filesystem::is_empty('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) succeeded: true
filesystem::directory_iterator('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) succeeded.

@StephanTLavavej StephanTLavavej self-assigned this Jan 17, 2024
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@MartinKuschnik
Copy link
Contributor Author

Thanks for the fix - I wouldn't have known how to do this myself! 😻

I pushed a commit to address my code review comments, and I double-checked that the fix still works. After running your New-VHD incantation, I captured my volume name:

C:\Temp>type meow.cpp
#include <exception>
#include <filesystem>
#include <print>
using namespace std;

int main() {
    println("_MSVC_STL_UPDATE: {}", _MSVC_STL_UPDATE);

    constexpr const char* volume = R"(\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\)";

    println("filesystem::exists('{}')) : {}", volume, filesystem::exists(volume));

    try {
        println("filesystem::is_empty('{}')) succeeded: {}", volume, filesystem::is_empty(volume));
    } catch (const exception& ex) {
        println("filesystem::is_empty('{}')) failed: {}", volume, ex.what());
    }

    try {
        const filesystem::directory_iterator it(volume);
        println("filesystem::directory_iterator('{}')) succeeded.", volume);
    } catch (const exception& ex) {
        println("filesystem::directory_iterator('{}')) failed: {}", volume, ex.what());
    }
}

Then reproed the bug with VS 2022 17.9 Preview 2.1:

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /MTd /Od meow.cpp && meow
meow.cpp
_MSVC_STL_UPDATE: 202310
filesystem::exists('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) : true
filesystem::is_empty('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) failed: is_empty: The system cannot find the file specified.: "\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\"
filesystem::directory_iterator('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) failed: directory_iterator::directory_iterator: The system cannot find the file specified.: "\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\"

And verified the fix:

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /MTd /Od meow.cpp && meow
meow.cpp
_MSVC_STL_UPDATE: 202401
filesystem::exists('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) : true
filesystem::is_empty('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) succeeded: true
filesystem::directory_iterator('\\?\Volume{0b4e8604-2b5f-418d-b1b4-189ca77f2ba1}\')) succeeded.

Thanks for giving details hints and fixing it yourself. Next time i can provide a better code based on your code review points.

@StephanTLavavej StephanTLavavej merged commit a0c2de5 into microsoft:main Jan 17, 2024
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks again for fixing this bug and congratulations on your first microsoft/STL commit! 🎉 😻 🛠️

This is expected to ship in VS 2022 17.10 Preview 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working filesystem C++17 filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<filesystem>: directory_iterator(...) and is_empty(...) not working with empty volumes
3 participants