ofToDataPath returning std::filesystem::path

[roymacdonald]

So,
on windows I get an error when trying to automatically convert from std::filesystem::path to string

and I have found it being an issue with ofxCv

@artificiel @ofTheo @dimitre

Although it compiles fine on macos.

[artificiel]

this happens because of an seemingly complicated (according to stack overflow etc) to navigate number of parameters relating to the setup of the IDE project options, encoding, manifest, BOM, filesystem, etc, the implicit conversion done by ofToDataPath's argument (which takes a path) will upgrade to wstring on Windows, even if it's explicitly passed a narrow std::string. (this is yet another side quest (testing) but it seems the OF tests in GitHub are setup in a way that does not trigger this problem, and/or the above test should be added).

so it's trying to convert wstring to a string in order to assign and not finding that converter.

don't have windows here; can you splice this in ofFileUtils.h at 1229 (just after ofToDataPath):

inline of::filesystem::path ofToDataPath2(std::string str, bool absolute = false) {
	return ofToDataPath(of::filesystem::path(std::string(str)), absolute);
}

and try your failure with ofToDataPath2? if that works we can work around by explicitly pushing std::string into filesystem::path. (and a potential "future" OF app that wants to be unicode-windows-friendly will anyway have to carefully use std::wstring or ::path and fall into the non-converted signature).

[NB the last phrase is another argument to "raise awareness of path vs string": if we expect some mac-developed app to compile in windows and support wide unicode, it means std::string is never used as a path container, even though on Mac it does not make an actual difference]

[so to be clear, std::string should never be used on windows for a path containers, but obviously legacy-compatibility requires a solution]

[roymacdonald]

@artificiel
I tested the snippet of code you posted and it does not work. same error.
the problem is implicitly converting from of::filesystem::path to std::string, not the other way around. Actually the problem seems to be that there is no implicit conversion from std::wstring to std::string.

I think that at least in the case of ofToDataPath and any class where the return type of a function which used to be std::string and got changed to of::filesystem::path we should roll back to std::string until we find a better solution as this will break on windows a lot of things. As I said elsewhere, we rollback the master branch and we make a dev branch where we have this changes and we test those.

[artificiel]

@roymacdonald as I mentioned elsewhere as far as management of /master goes i am not going to argue against. I responded to this issue as I was under the impression the above had been tested a while ago within the OF tests. it seems either that basic case (which is obviously required) is not part of the tests, or the tests are setup in such a way that the MSVC behaviour is different — it must be ensured that the case is effectively tested and should have failed the PR. I don't know enough about the setup of these GitHub automated testing tasks to contribute effectively on the topic.

[roymacdonald]

@artificiel I am not sure either, but it should be tested and as it is such a basic thing. and I can tell from a project in which I am working now, which I had to move from macos to windows, that it is actually incredibly annoying to deal with. I will take a closer look at the tests being done. @ofTheo any idea where/which are those tests?

[artificiel]

@roymacdonald the tests are here: https://github.com/openframeworks/openFrameworks/blob/master/tests/utils/fileUtils/src/main.cpp

(where i can't help is how are configured the gtihub CI automated builds, as vs2022-* passes all tests currently)

[danoli3]

Sounds like just need to fix ofxCv

…

On Mon, 14 Oct 2024 at 7:06 am, alexandre burton ***@***.***> wrote: @roymacdonald <https://github.com/roymacdonald> the tests are here: https://github.com/openframeworks/openFrameworks/blob/master/tests/utils/fileUtils/src/main.cpp (where i can't help is how are configured the gtihub CI automated builds, as vs2022-* passes all tests currently) — Reply to this email directly, view it on GitHub <#8143 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGK2HALUQWEPXIRJAW2SFLZ3LHCTAVCNFSM6AAAAABPZTWDTOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBZGEYDONBRGE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[artificiel]

@danoli3 ofxCv is an important one but there is plenty of code that contains code like std::string s = ofToDataPath(""), or object.open(ofToDataPath("")) where .open() expects a std::string. that will never compile on MSVC as the internal representation of datapath is always wide even if the path does not require it — as far as i can tell, which includes perusing the source of MS's implementation:
https://github.com/microsoft/STL/blob/926d458f82ae1711d4e92c0341f541a520ef6198/stl/inc/filesystem#L633-L635
(i say "never" -- there are numerous complaints around that behaviour that hinders the cross-platform design of std::filesystem::path (as we encounter), but let's not count on that).

perhaps a compatibility mode with a flag such as #if defined(DATAPATH_AS_STRING) && defined(OF_OS_WINDOWS) which would force .string() before returning the path? all functions taking a ::path should be happy with a std::string even on windows. the drawbacks are immaterial to the use case which would be existing code that anyway does not support wide unicode on windows.

[ofTheo]

Edit: nevermind.
Going to look at other solutions...

Below might still be feasible:

This only addresses ofToDataPath - if we wanted to address all functions with return types of of::filesystem::path we would maybe need a custom datatype that could have = operator overloads.

ie: ofFilePath ofToDataPath( of::filesystem::path, bool makeAbsolute );

[ofTheo]

I agree in terms of shorterm fixes this should probably be reverted until we can get a workable solution for Windows.
This would literally break almost every app on Windows at this point.

[roymacdonald]

Edit: nevermind. Going to look at other solutions...

Below might still be feasible:

This only addresses ofToDataPath - if we wanted to address all functions with return types of of::filesystem::path we would maybe need a custom datatype that could have = operator overloads.

ie: ofFilePath ofToDataPath( of::filesystem::path, bool makeAbsolute );

I was thinking about something around the same idea. Since we already are aliasing std::filesystem::path into of::filesystem::path we could just use a preprocessor define so only in windows it returns this new datatype with overloaded = operator, otherwise it is just the alias.

[roymacdonald]

I agree in terms of shorterm fixes this should probably be reverted until we can get a workable solution for Windows. This would literally break almost every app on Windows at this point.

I am currently developing on windows so I can do this changes, test and make a PR

[roymacdonald]

So, I was checking the git commit history and there are a bunch of commits which made the switch to using of::filesystem::path instead of std::string.

What would be the best strategy?
Options:

• reverting those commits:
  • Pros:
    • Easy.
  • Cons:
    * Might mess up things added after those commits.
• making the changes by hand and making a new commit
  • Pros:
    • Should not interfere with what we currently have
  • Cons:
    * more work required.

what do you think @ofTheo @artificiel @danoli3

[ofTheo]

@roymacdonald I think there was a point where the current ofToDataPath with named ofToDataPathFS and then ofToDataPath was:

//--------------------------------------------------
std::string ofToDataPath(const fs::path & path, bool makeAbsolute){
	return ofPathToString(ofToDataPathFS(path, makeAbsolute));
}

I think if we can PR back to that change even as a new commit but with just the rename of ofToDataPath to ofToDataPathFS and the above function - that might be all that is needed.

@dimitre would probably be the best to ask though as he's been working the most on this.

[artificiel]

yes at some point there was 2 versions of each of all these FS-suffxied paths methods and as i understand the process, they propagated and got progressively combined as tests were passing.

changing ofToDataPath to return std::string on windows (or maybe more precisely MSVC) should solve most problems for now.

but, it reallly should be ensured the VS builds are failing (upon std::string a = ofToDataPath("a");) — as of now the tests are not failing which in itself is a failure...

[dimitre]

I can't opinion on reverting because I'm still favoring the idea of moving to fs::path and ofPathToString
And adequating code to use auto so we don't have unintended conversion from wide string to narrow