SYCL builds of UR fail due to undefined reference to std::filesystem

[bmyates]

After addition in #439 some SYCL builds of UR are failing with undefined reference to std::filesystem::__cxx11::path::_M_split_cmpts()`

See this comment:: intel/llvm#9053 (comment)

[kbenzie]

@bmyates I'd ideally like to replicate this build environment in our CI so we can catch these issues before they reach SYCL, is it documented somewhere?

[pbalcer]

We could rip out the use of std::filesystem and replace it with two platform-specific implementations. It's not used for much. But I'm not sure if that's a good solution. UR's README states that it requires C++17, and that also means a C++ standard library that adequately implements its features. If we can't rely on that, and we do truly need to support platforms with old compilers, I'd prefer we revert to C++14 instead of playing endless whack-a-mole to support compilers/standard libraries with bugs or partial implementations.

It's not reasonable to expect that UR (or any project) is bug-compatible with every compiler and standard library version. We need to have stable target platforms (e.g., whatever ships in some stable versions of RHEL, SUSE, and Debian).
How does SYCL approach this? The getting started guide only lists GCC version 7.1.0 or later.

Also, I found this, which says that std::filesystem on GCC 8 requires adding -lstdc++fs. I think this should solve the immediate problem.

[kbenzie]

The C++17 requirement is inherited from upstream LLVM, I still think its reasonable for UR to also require it. I'd like to avoid reverting to C++14 if possible as <filesystem> is not the only instance of C++17 already in use.

This issue is frustrating though and I'd like to avoid getting bug reports from CI pipelines I'm not aware of moving forwards.

It's not reasonable to expect that UR (or any project) is bug-compatible with every compiler and standard library version. We need to have stable target platforms (e.g., whatever ships in some stable versions of RHEL, SUSE, and Debian).
How does SYCL approach this? The getting started guide only lists GCC version 7.1.0 or later.

Totally agree that we need to have a list of supported platforms, those at least need to line up with the ones in DPC++. Once we have that we can expand our CI and acceptance criteria for PRs to avoid recurrances moving forwards.

Also, I found this, which says that std::filesystem on GCC 8 requires adding -lstdc++fs. I think this should solve the immediate problem.

These lines in the root CMakeLists.txt should be setting -lstdc++fs globally for all UR targets but the link step for ur_loader doesn't have this option. I'll look into this.

[bmyates]

This issue is frustrating though and I'd like to avoid getting bug reports from CI pipelines I'm not aware of moving forwards.

I agree, it is frustrating for me as well. I have requested a list of required platforms from SYCL but have not received an answer yet.

[andykaylor]

I ran into this problem trying to build the SYCL compiler from the intel/llvm repository. I am using a RHEL8 system with gcc 8.3.1. It doesn't look like it's caused by a lack of C++17

[kbenzie]

RHEL 8 is be based on Fedora 28 and I've managed to reproduce this error there, working on a fix.

[kbenzie]

@bmyates / @andykaylor could you please verify the fix in #456 works as expected on the fialing RHEL builds?

[andykaylor]

@kbenzie #456 does fix the problem for me. Thanks!

[kbenzie]

Thanks @andykaylor, I'll merged the now.

I've created #457 to cover adding CI to try and avoid this reoccuring in future.