Raw: make the output directory creation more robust. - #6458
Conversation
Turns out that `std::filesystem::create_directories` on some platforms/compilers does not return true even when actually creating the directory.
|
|
||
| void o2::raw::assertOutputDirectory(std::string_view outDirName) | ||
| { | ||
| std::filesystem::create_directories(outDirName); |
There was a problem hiding this comment.
I think the logic here should be:
if (!exists) {
#ifdef MACOS
create();
if (!exists) error;
#else
if (!create()) error;
#endif
}
with some comment that this is broken on mac OS currently and that the workaround should be reverted once it works. I like merging all the checks into this one function.
There was a problem hiding this comment.
Well, yes, that's another option. But I believe the check should be done on the compiler, not the platform, as I think the issue is more a Clang vs GCC one (I understand we're only using Clang on Mac, but the intent of the check would be clearer that way)
There was a problem hiding this comment.
Yes, sure, my point was just we should document why it is done like this, together with a comment that it should be cleaned up. Is there actually any reference that this is broken on Mac / clang? Clang is usually well-tested, so I would be surprised if create_directories would be misbehaving.
There was a problem hiding this comment.
Would have to dig a reference if there is one, but can be seen in e.g. https://godbolt.org/z/qcE5fMq36
There was a problem hiding this comment.
OK, then it is most likely a bug in clang?
Can you open a bug report? The clang developers are usually quite responsive.
There was a problem hiding this comment.
And still, could we change it to have the exists check first, and then add a comment with a todo that this is a workaround due to a clang bug?
There was a problem hiding this comment.
I've just added a commit (to be squashed on merge) in that respect : would that be ok for you ?
davidrohr
left a comment
There was a problem hiding this comment.
perfect, thx, will squash-merge once CI is through
|
@aphecetche : This seems to be a genuine error: |
* Raw: make the output directory creation more robust. Turns out that `std::filesystem::create_directories` on some platforms/compilers does not return true even when actually creating the directory. * assertOutputDirectory: change logic and add comment * put the ending brace in the right place...
Turns out that
std::filesystem::create_directorieson some platforms/compilers does not return true even when actuallycreating the directory. So this PR changes a bit the logic used to force creation of output directory : call
std::filesystem::create_directoriesbut do not rely on the boolean return value. Make an explicitstd::filesystem::existscall instead.Discovered that while trying to get the FST working on macOS. The symptom was that the rawXXX.log showed a fatal due to the impossibility to create the raw/XXX directory. But in fact the directory was created just fine.