Skip to content

build: require libc++ and clang headers #946

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

Merged
merged 1 commit into from
Jul 29, 2025

Conversation

mizvekov
Copy link
Collaborator

@mizvekov mizvekov commented Jul 28, 2025

Make CMakeLists.txt enforce the requirement for libc++ and the clang headers.

Otherwise, it's possible for configuration to succeed but produce a non-functional mrdocs, where the tests fail in non-obvious ways.

@mizvekov mizvekov self-assigned this Jul 28, 2025
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://946.mrdocs.prtest2.cppalliance.org/index.html

Copy link

codecov bot commented Jul 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.42%. Comparing base (0a751ac) to head (cde1ab6).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #946   +/-   ##
========================================
  Coverage    84.42%   84.42%           
========================================
  Files          190      190           
  Lines        20565    20566    +1     
========================================
+ Hits         17362    17363    +1     
  Misses        3203     3203           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alandefreitas
Copy link
Collaborator

alandefreitas commented Jul 28, 2025

Make the CMakeLists.txt enforce that requirement.

The code might be flawed but it already enforces this requirement. This PR only changes the condition (it should be unconditional anyway).

@mizvekov mizvekov force-pushed the cmake-require-system-includes branch from 3206c26 to d45d0a5 Compare July 29, 2025 04:41
@mizvekov
Copy link
Collaborator Author

Make the CMakeLists.txt enforce that requirement.

The code might be flawed but it already enforces this requirement. This PR only changes the condition (it should be unconditional anyway).

You can make both calls to find_package (LLVM, Clang) succeed, without specifying LLVM_ROOT at all, by specifying LLVM_DIR and Clang_DIR instead.

This makes configuration succeed, but it results in a non-functional installation and tests.

If you mean we should just unconditionally require those things instead, that also works for me.

@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://946.mrdocs.prtest2.cppalliance.org/index.html

@alandefreitas
Copy link
Collaborator

You can make both calls to find_package (LLVM, Clang) succeed, without specifying LLVM_ROOT at all, by specifying LLVM_DIR and Clang_DIR instead

Yes. That's what I mean by "The code might be flawed".

In other words, this is more of a fix than a feature.

Also related, cmake is an invalid conventional commit type. The type would be build.

If you mean we should just unconditionally require those things instead, that also works for me

Yes. We don't need

if (MRDOCS_BUILD_TESTS OR MRDOCS_INSTALL)

There's no case where MrDocs does not depend on these headers.

To be honest, the condition MRDOCS_BUILD_TESTS OR MRDOCS_INSTALL is not that far from if (TRUE) in practice.

@mizvekov mizvekov force-pushed the cmake-require-system-includes branch from d45d0a5 to f4a61f8 Compare July 29, 2025 05:58
@mizvekov
Copy link
Collaborator Author

In other words, this is more of a fix than a feature.

Yes, that was the intention here, this is strictly a fix.

@mizvekov mizvekov changed the title cmake: require libc++ and clang headers when building with tests or install build: require libc++ and clang headers when building with tests or install Jul 29, 2025
@mizvekov mizvekov changed the title build: require libc++ and clang headers when building with tests or install build: require libc++ and clang headers Jul 29, 2025
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://946.mrdocs.prtest2.cppalliance.org/index.html

Make CMakeLists.txt enforce the requirement for libc++ and the clang
headers.

Otherwise, it's possible for configuration to succeed but produce
a non-functional mrdocs, where the tests fail in non-obvious ways.
@mizvekov mizvekov force-pushed the cmake-require-system-includes branch from f4a61f8 to cde1ab6 Compare July 29, 2025 16:03
@cppalliance-bot
Copy link

An automated preview of the documentation is available at https://946.mrdocs.prtest2.cppalliance.org/index.html

@mizvekov mizvekov marked this pull request as ready for review July 29, 2025 16:48
@mizvekov mizvekov requested a review from alandefreitas July 29, 2025 16:48
@alandefreitas alandefreitas merged commit 6e5f80c into develop Jul 29, 2025
34 checks passed
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