-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docs: detect version of clang++ #848
Conversation
An automated preview of the documentation is available at https://848.url.prtest2.cppalliance.org/libs/url/doc/html/index.html |
GCOVR code coverage report https://848.url.prtest2.cppalliance.org/gcovr/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #848 +/- ##
========================================
Coverage 99.21% 99.21%
========================================
Files 157 157
Lines 8422 8422
========================================
Hits 8356 8356
Misses 66 66 Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest issue with my initial comment is whether clang++-18 is actually available on the machine that builds the preview. Is it?
set -e | ||
|
||
# If CXX was not already set, then determine the newest clang. | ||
if [ ! -n "$CXX" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow already sets CXX and CC. Is this change to ensure it works in the container that builds the preview?
Yes, in the container that builds the previews. Another topic.. this section:
Can be simplified to :
If |
Nice :)
Yes. I opened an issue in MrDocs to address that: cppalliance/mrdocs#644 When this changes, any compiler can run cmake to generate the compile_commands.json, but the standard library headers will ultimately come from MrDocs so there will never be any conflict between the mrdocs compiler and the standard library headers.
Yes. This logic is already in the javascript script that generates the reference pages. It prefers clang, then GCC, than MSVC. Line 98 in 95e50b8
The problem we have is it fails to find
Yes. That's also the case because the script that generates the reference pages also checks these environment variables. The problem for now is more about clang++-18 not being available or when it can't be found because CXX is not set. But it already identifies CXX. We should be good as long as clang++-18 is available. I'll include some logic to look for clang++-\d+ and gcc-\d+ in the extension. |
It sounds like you will solve the problem in various other (better) ways within Mr. Docs itself, and so on. OK.
The previews container has clang++-18. But was getting The logic in this pull request searches for the newest clang, rather than specifically 18. Soon it will happen that clang-20 is available while clang-18 is missing. That case would already be handled. But if you will solve the problem more correctly elsewhere, that is great. |
Yes. There are many levels to it. We had this problem finding the compiler, which I was already fixing in the Antora extension, so it finds these latest versioned executables by itself. MrDocs currently needs the compiler to run cmake, but there's potential for improvement. In the future, MrDocs might be able to use itself as the compiler. However, the other issue is with the standard library, for which we also have an issue in the MrDocs repo.
Nice. That's the only thing I needed clarification on. As long as the compiler is available, #843 should solve the issue. The PR is already receiving previews again: https://843.urlantora.prtest2.cppalliance.org/site/index.html
Yes. The solution implemented in the extension is a little more elegant because it's at a lower level and because in bash we would "need" to look for clang-40, clang-39, ... |
Obsoleted by #843 |
@alandefreitas wrote: "@sdarwin The Antora workflow that generates the preview will probably break again." Yes the same issue occurred on http_proto. This fix allows
build_antora.sh
to be run locally, or from another location, not depending on inputs from CI.