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

WIP: use fs::path for app #3125

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP: use fs::path for app #3125

wants to merge 1 commit into from

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Jan 17, 2025

This fails on both Linux and Windows. The latter is a compilation issue. I'm not sure how to fix.

Linux fails tests because sstreams add quotes around a path.

I assume the simplest way to fix that would be to use std::format and output that.

@neheb
Copy link
Collaborator Author

neheb commented Jan 17, 2025

oh nvm:

[__cpp_lib_format_path](https://en.cppreference.com/w/cpp/feature_test#cpp_lib_format_path) 	[202403L](https://en.cppreference.com/w/cpp/compiler_support/26#cpp_lib_format_path_202403L) 	(C++26) 	Formatting of [std::filesystem::path](https://en.cppreference.com/w/cpp/filesystem/path)

@kmilos
Copy link
Collaborator

kmilos commented Jan 17, 2025

At this point, honestly, why really bother w/ wstring?

Windows 10 & 11 make up 97% of the installs:

https://gs.statcounter.com/os-version-market-share/windows/desktop/worldwide
https://www.statista.com/statistics/993868/worldwide-windows-operating-system-market-share/

Windows 7 still hanging about 2.5% was EOL'd 5 years ago. Windows 8.1 was EOL'd 2 years ago, and has a negligible share...

@neheb
Copy link
Collaborator Author

neheb commented Jan 17, 2025

std::filesystem uses wstring for its paths.

@neheb neheb force-pushed the 2 branch 2 times, most recently from db49b5a to 6c98e2c Compare January 18, 2025 02:20
@kmilos
Copy link
Collaborator

kmilos commented Jan 18, 2025

std::filesystem uses wstring

While it does implement/provide those overloads for legacy reasons, the wstring code path is not used/seen at all if the app is running in the UTF-8 code page.

@neheb
Copy link
Collaborator Author

neheb commented Jan 19, 2025

@kmilos how do I enable this std::filesystem == std::string mode?

@neheb
Copy link
Collaborator Author

neheb commented Jan 19, 2025

@kmilos I don't think this is right.

On meson, I just tried doing a build with MSVCRT:

meson.build:23:4: ERROR: Problem encountered: Non UCRT MinGW is unsupported. Please update toolchain

meaning std::filesystem uses std::wstring on UCRT64.

@neheb neheb force-pushed the 2 branch 2 times, most recently from 122a856 to 31788a8 Compare January 19, 2025 23:53
@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

It does on MSVCRT, but does not on UCRT.

meaning std::filesystem uses std::wstring on UCRT64

It does not mean this. We explicitly dropped support for building on MSVCRT when we dropped wstring.

@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

how do I enable this std::filesystem == std::string mode?

It is already enabled on the main branch: both CMake and meson build w/ UCRT and attach the UTF-8 code page manifest to the exiv2 app and other executables. Other clients of the library are expected to do the same.

See #2951 again.

@neheb
Copy link
Collaborator Author

neheb commented Jan 20, 2025

I'm specifically asking about std::filesystem.

@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

AFAIK, there is nothing specific about std::filesystem. If the app is built w/ UCRT and runs in UTF-8 code page, it's the same as on non-Windows platforms, it's seamless and only string/char is ever used.

But I will confirm this once I get access to my UCRT64 machine...

@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

@neheb I stand corrected, apologies! It does look like (at least GNU and LLVM) std::filesystem::path is exclusively wchar_t on Windows regardless of underlying C lib (and I guess the same is w/ Visual Studio)...

That makes things more complicated indeed, and not that seamless as the UTF-8 switch by UCRT promised... 😕

@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

However, although the underlying type is wchar_t on Windows, it would seem the std::filesystem should still handle any conversion automagically and path can be constructed and accessed by any type:

https://en.cppreference.com/w/cpp/filesystem/path/string
https://en.cppreference.com/w/cpp/filesystem/path/path
https://en.cppreference.com/w/cpp/filesystem/path/u8path

I.e. it looks like you can't use fs::path::c_str() directly (w/o even more #ifdefs) but should have a UTF-8 conversion through either fs::path::string() or fs::path::u8string()?

@neheb
Copy link
Collaborator Author

neheb commented Jan 20, 2025

Right. The whole reason wstring was brought back was to get it to compile on Windows with fs::path. Although at this point, it’s a lot of changes to action.cpp

@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

The whole reason wstring was brought back was to get it to compile on Windows with fs::path

It was compiling just fine w/o it, the CI was green (apart from OSS-Fuzz). It was brought back for the wchar_t API to clients.

@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

Looks like they really went to town in C++20 w/ UTF-8 and std::filesystem breakage:

https://stackoverflow.com/a/59994164 (see also the first link by the author)

Signed-off-by: Rosen Penev <[email protected]>
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.

2 participants