Skip to content

[cmake][interop] add dynamic check for libstdc++fs#21138

Closed
aaronj0 wants to merge 1 commit intoroot-project:masterfrom
aaronj0:dynamic-check-stdcxxfs
Closed

[cmake][interop] add dynamic check for libstdc++fs#21138
aaronj0 wants to merge 1 commit intoroot-project:masterfrom
aaronj0:dynamic-check-stdcxxfs

Conversation

@aaronj0
Copy link
Contributor

@aaronj0 aaronj0 commented Feb 3, 2026

Based on 957ac33: Looking at the compiler version is not enough to know if we really need to link against stdc++fs:
"Whether linking with libstdc++fs is needed depends on the version of libstdc++, not on the compiler. For example, it is possible to use an older library with a newer compiler, or with another compiler that follows a different versioning scheme (for example Clang)."
594a2b8 broke some Clang builds as the need to link against stdc++fs was not a GCC issue, but to do with the libstdc++ version. This adds a dynamic check in the same fashion as in ROOT, but in CppInterOp so that standalone builds do not suffer from the same issue.

The same dynamic check if a source compiles doesn't seem to work in CppInterOp's cmake. I will investigate further, for now updating the check to use ROOT_NEED_STDCXXFS

@hahnjo can you test if this commit fixes your build?

@aaronj0 aaronj0 requested a review from bellenot as a code owner February 3, 2026 19:39
@aaronj0 aaronj0 requested review from guitargeek and hahnjo February 3, 2026 19:39
@aaronj0 aaronj0 force-pushed the dynamic-check-stdcxxfs branch 2 times, most recently from 03f6efe to bd35dfe Compare February 3, 2026 19:59
@aaronj0 aaronj0 force-pushed the dynamic-check-stdcxxfs branch from 7702434 to 28a646c Compare February 3, 2026 20:30
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks.

@github-actions
Copy link

github-actions bot commented Feb 3, 2026

Test Results

    22 files      22 suites   3d 9h 0m 33s ⏱️
 3 779 tests  3 778 ✅ 0 💤 1 ❌
75 161 runs  75 160 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 28a646c.

@hahnjo
Copy link
Member

hahnjo commented Feb 4, 2026

I tried to comment on the old commit, but that doesn't seem to show up 😒

The probable reason the check didn't work in CppInterOp integrated into ROOT is that we never reset CMAKE_REQUIRED_LIBRARIES in cmake/modules/SearchInstalledSoftware.cmake. So the variable is still there and the HAVE_NATIVE_FILESYSTEM already passes.

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from #20774 (comment):

Looking at the code again, the only place using <filesystem> is https://github.com/compiler-research/CppInterOp/blob/4cc7ff5e9af52e6535d3c8f33e8fce7b6c8120b4/lib/CppInterOp/CppInterOp.cpp#L3357-L3358. Instead I think you should just call llvm::sys::fs::is_directory, which you are already using throughout the file...

@aaronj0
Copy link
Contributor Author

aaronj0 commented Feb 4, 2026

Closing in favour of compiler-research/CppInterOp#797 that should no longer require linking against stdc++fs

@aaronj0 aaronj0 closed this Feb 4, 2026
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.

3 participants