Add native macOS/AppleClang support with CI workflow#2630
Add native macOS/AppleClang support with CI workflow#2630tanmay-9 wants to merge 60 commits intoad-freiburg:masterfrom
Conversation
… for Cpack version
…es to exclude their install rules from being called when running cpack
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2630 +/- ##
=======================================
Coverage 91.60% 91.60%
=======================================
Files 483 483
Lines 41360 41362 +2
Branches 5493 5493
=======================================
+ Hits 37886 37888 +2
Misses 1897 1897
Partials 1577 1577 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ang/LLVM Clang compiler crash with packaged_task + Boost.Asio
tanmay-9
force-pushed
the
apple-native-build
branch
from
50c0ce8 to
23a7fc4
Compare
tanmay-9
force-pushed
the
apple-native-build
branch
from
23a7fc4 to
751abb0
Compare
remove unsupported (EOL) ubuntu releases and add debian releases
…constructing VERSION_SUFFIX
This reverts commit 2d7fa61.
…ich has a deleted copy constructor) on AppleClang.
tanmay-9
changed the title
hannahbast
requested review from
RobinTF,
Copilot and
joka921
and removed request for
joka921
RobinTF
left a comment
RobinTF
left a comment
There was a problem hiding this comment.
Looks good, mostly some minor comments or thoughts.
src/engine/GroupByImpl.cpp
Outdated
| blockStart, blockEnd, | ||
| ¤tLocalVocab_, groupByCols_); | ||
| // This processes the whole block in batches if possible. | ||
| // Note: Use `template` keyword for dependent template name. |
There was a problem hiding this comment.
The note is a little bit redundant. That's what the template keyword is for in this scenario.
|
|
||
| AD_LOG_INFO << EMPH_ON << "QLever index builder, compiled on " | ||
| AD_LOG_INFO << EMPH_ON << "QLever index builder " | ||
| << qlever::version::ProjectVersion << ", compiled on " |
There was a problem hiding this comment.
This seems to be an unrelated change, I assume this is intentional?
test/util/AsyncTestHelpers.h
Outdated
| promise->set_exception(std::current_exception()); | ||
| } | ||
| }); | ||
| return fut; |
There was a problem hiding this comment.
This is why we can't have nice things. If it's an compiler crash, did you try just assigning the packaged task to an extra variable, or playing around with scopes? Sometimes this helps. Otherwise your workaround is also fine, but I'd like to avoid the verbosity if possible.
| // Use index-based iteration instead of ranges::zip_view to avoid | ||
| // copying RowReference (which has a deleted copy constructor) on | ||
| // AppleClang. |
There was a problem hiding this comment.
This seems ... odd. zip view shouldn't copy anything. Did you try wrapping stuff in ql::ranges::ref_view? This should let you explicitly avoid copies.
RobinTF
left a comment
RobinTF
left a comment
There was a problem hiding this comment.
Thank you very much for your changes, I still have 2 questions for you.
| for (const auto& [rightId, leftCol] : | ||
| ::ranges::zip_view(rightRow, leftColumns)) { | ||
| ::ranges::views::zip(ql::ranges::ref_view{rightRow}, | ||
| ql::ranges::ref_view{leftColumns})) { |
There was a problem hiding this comment.
If I understand your comment correctly, it's only the rightRow that needs the wrapper? leftColumns should be a span, which is cheap to copy regardless.
test/util/AsyncTestHelpers.h
Outdated
| auto fut = task.get_future(); | ||
| net::post(*ioContext, std::move(task)); | ||
| return fut; |
There was a problem hiding this comment.
So just to be sure, as soon as you turn this into
| auto fut = task.get_future(); | |
| net::post(*ioContext, std::move(task)); | |
| return fut; | |
| return net::post(*ioContext, std::move(task)); |
or
| auto fut = task.get_future(); | |
| net::post(*ioContext, std::move(task)); | |
| return fut; | |
| auto fut = net::post(*ioContext, std::move(task)); | |
| return fut; |
The compiler crashes again, right? If not, both of these options would be more desirable.
Overview
Conformance check failed ❌Test Status Changes 📊
|
|
3 participants
Adds native AppleClang/macOS compatibility and a GitHub Actions workflow for CI on macOS. Additionally, add project version to the logs generated when executing
qlever-indexorqlever-server.Changes
Build System
Code Fixes
AppleClang enforces stricter C++ standard compliance than GCC, requiring several fixes:
Template disambiguation: Added
templatekeyword for dependent template member calls.Range adaptor compatibility: Wrapped range arguments in
allView()before passing totransform_viewto ensure proper view semantics across implementations.Non-copyable types in
zip_view: Useql::ranges::ref_viewto avoid copying element types (with a deleted copy constructor) on AppleClang.packaged_task+ Boost.Asio: Use a named variable to work around AppleClang compiler crash when passing a temporarypackaged_taskdirectly tonet::post.