cling: changes needed on MacOS to avoid G__Core.cxx generation failures complaining about time_point #16494
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…es complaining about time_point
On one installation of MacOS with Sonoma 14.6.1 and XCode 15.4 and clang-1500.3.9.4
(not sure about the xcode command line tool version), the generation of
G__Core.cxx failed until we applied the change in this commit.
On another installation of MacOS with the same version numbers the
change was not needed but the change did not hurt either.
```
In module 'std' imported from input_line_1:1:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk/usr/include/c++/v1/__chrono/time_point.h:34:52:
error: missing '#include <__chrono/file_clock.h>'; '_FilesystemClock'
must be defined before it is used
template <class _Clock, class _Duration = typename _Clock::duration>
^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk/usr/include/c++/v1/__chrono/time_point.h:34:1:
note: in instantiation of default argument for
'time_point<std::filesystem::_FilesystemClock>' required here
template <class _Clock, class _Duration = typename _Clock::duration>
^~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk/usr/include/c++/v1/__chrono/file_clock.h:51:8:
note: definition here is not reachable
struct _FilesystemClock {
^
```
Test Results 13 files 13 suites 3d 9h 14m 26s ⏱️ For more details on these failures, see this check. Results for commit ac6d9f4. ♻️ This comment has been updated with latest results. |
|
Is this done upstream? I'm not feeling comfortable changing the copied modulemaps too much, we have to move to a more dynamic scheme to not break on changes by Apple, and every local modification makes this harder. So far, we "only" added a surrounding |
Upstream is taking a different route in llvm/llvm-project#107638 Let's hope soon we can drop this huge and messy workaround we have... |
That hasn't landed yet and even if it did today, it's still multiple versions of SDK down the road. So why are we seeing problems now that nobody else has? And where do we see them at all, because our CI is green? |
Simply because our use of C++ modules is on the bleeding edge. |
|
That doesn't really answer any of my two questions: There are other parties than us using the libc++ modulemap, notably Apple. Why don't they see this problem? And what is special about the two installations alluded to in the commit message, when it's working in the CI? |
I am not sure how to answer them. Apple Swift uses that modulemap for something to do C++ interop with. I do not know the details but Swift-C++ is probably not the go-to choice for swift developers most of the time. I am not sure what's the answer to your second question. That's for @pcanal to answer but the answer does not really matters. Before it did not work after this patch works for his setup. |
|
It does matter a lot because I'm totally not happy that we are starting to patch the modulemap that we copied from Apple's distribution of libc++. If it's the only possibility, then we can do it but I'm not able to assess that without knowing why it doesn't work in Philippe's while many other installations, including our CI nodes, are completely fine. |
I am not happy either. However, I do not see what we are after here? He, just like any other random user, installed something from Apple and thing broke. The options here are basically two: a) tell the user he is wrong and not fix it which is silly; b) fix it. The only place we could fix it is the modulemap as that's the only place we have write access to :) |
|
@stognini @pcanal here are some commands that should get us a decent overview of the compiler + SDK setup: Output from `macphsft18` (macOS 14.7)Output from `macphsft31` (macOS 15.0)Output from `macphsft14` (macOS 15.1 beta)Can you maybe post the output from your systems? Watch out for |
|
Here is my output: |
|
And mine for the record -- confirmed to work without the patch with commit 58a96e2 |
|
On my machine the build with |
|
So I reinstalled the command line tools (see for example https://root-forum.cern.ch/t/root-on-macos-15-xcode-16/61716/9). |
|
Okay interesting, you have your SDKs in a different place. @stognini can you run |
|
Sure: |
|
@hahnjo ping |
|
It's still not clear to me why this change is needed in one particular configuration, ie what is different to all other developers building on macOS, our CI, ... Also I would to point out once more that patching our modulemap for macOS is not at all sustainable. In the past, we never had problems because we just took whatever Apple shipped with libc++. We started having a copy to revert an upstream change and have a single big |
|
There has been some progress in llvm/llvm-project#86193, however, as far as I can tell it is not sufficient... |
This is the upstream part, which is important. However, we also discussed that we want an automatic solution downstream, ie not maintain a copy of the modulemap but instead create it dynamically from the file found in the SDK. This would solve all recent problems because it's again on Apple / libc++ to provide a valid modulemap. |
This was referenced Jan 29, 2025
I'd like to avoid that as a long term solution because that's a libc++ problem... There were steps in the right direction, however, the use-case coverage we have seems to exceed theirs... |
I'm confused, I thought this was the agreed short-term solution that at least doesn't require patching every time there is a new SDK. What is your proposal to reduce problems until it is fixed upstream, released by Apple, and all older versions are EOL? |
I did look into that very briefly. It won't be a direct copy-paste from the SDK as we have some patches on top of it like: 8045591, but should be doable in CMake. |
1 task
|
Related: #18306 (comment) (and #18235 for the comment above) |
|
hello. We still have this issue on master, with a new SDK! |
Merged
2 tasks
|
After a bit more digging, I was able to make a reproducer with AppleClang. I think it is a problem in Apple’s current libc++ module-map, not a ROOT problem. #include <filesystem>
int main() {
std::filesystem::file_time_type::clock::now();
return 0;
}
We get similar errors: #18515 - fixes this without changing modulemaps |
|
Is the pr deliberately avoiding a fix is the modulemap? |
Yes, but just an alternative fix. Fixing module-map as per this also looks ok to me, since this is already an issue we have with clang on Apple. |
In that case we can re-export |
Thanks for investigating. The next question would be if this is Apple's fault or present in upstream libc++. Quoting my initial comment on this PR:
If that's the case, we should report it so it has a chance of getting properly fixed. If that has already happened, I'm ok with patching our copy of the modulemap but otherwise I'd prefer the workaround (with a reference to the upstream issue) because our primary goal must be to get back to using the installed modulemap on macOS. |
|
I was able to reproduce this with upstream libc++ and opened an issue here: llvm/llvm-project#138683 |
Thanks! Then in my opinion, we should go with the workaround in #18515 and add a reference to the upstream bug report. That way, we are independent of when upstream fixes the problem. |
|
Why not the modulemap change? |
So we can drop our copy of modulemap on our own timeline / potentially generate it from the installed one. |
I agree with the wish of dropping the copy, however, I do not think this is realistic unless we somehow add preliminary integration setup with root and libc++ which helps us detects problems introduced by libc++. Otherwise the change in the header files to achieve the same effect seems more like a workaround rather than a fix. We will need to somehow annotate these workaround and drop them otherwise we will be just polluting the source code to avoid patching the broken modulemap -- won't make it less broken, right? |
We lived with the installed modulemap on macOS for years. Our integration setup is
Yes, that's why I want to have a reference to the upstream bug. The reason I consider this superior to patching the modulemap is that the headers are ours, while the modulemap is a copy that we should really get rid of. |
|
If the majority prefers it, we can also go with the modulemap change right now (especially for the release), even though I'd like to argue once more that a workaround on our side looks like the better solution to me. If at some point this is the only modification blocking is from using the upstream modulemap, we can still revisit. |
|
I think this PR can be closed after merging #18515 |
|
Thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On one installation of MacOS with Sonoma 14.6.1 and XCode 15.4 and clang-1500.3.9.4
(not sure about the xcode command line tool version), the generation of
G__Core.cxx failed until we applied the change in this commit.
On another installation of MacOS with the same version numbers the
change was not needed but the change did not hurt either.