Skip to content
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

[c++] General CMake clean-up #2762

Merged
merged 9 commits into from
Jun 28, 2024
Merged

[c++] General CMake clean-up #2762

merged 9 commits into from
Jun 28, 2024

Conversation

jp-dark
Copy link
Collaborator

@jp-dark jp-dark commented Jun 27, 2024

Issue and/or context: Closes #2664

Changes:

  • Add options explicitly to targets instead of to everything. Note: this means compile options are no longer propagated to third-party libraries included with FetchContent and ExternalProject_Add.
  • Move find_package calls and coverage flags to base CMakeLists.txt instead of adding them twice.
  • Re-arrange some CMake calls to reduce redundancy and improve readability.
  • Remove unused GIT_COMMIT_HASH compiler definition.

We are only using the compile options on C++ libraries, so no need to
specify compile language. Using the generator expression for the
target based compile options was causing a CMake error.
Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.02%. Comparing base (9cfbdca) to head (b47cee7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2762      +/-   ##
==========================================
+ Coverage   89.87%   90.02%   +0.15%     
==========================================
  Files          37       37              
  Lines        3939     3939              
==========================================
+ Hits         3540     3546       +6     
+ Misses        399      393       -6     
Flag Coverage Δ
python 90.02% <ø> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 90.02% <ø> (+0.15%) ⬆️
libtiledbsoma ∅ <ø> (∅)

@jp-dark jp-dark marked this pull request as draft June 27, 2024 19:02
@jp-dark jp-dark changed the title [c++] Update CMake to use target-based properties [c++] General CMake clean-up Jun 27, 2024
Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

This seems to be functionally equivalent but uses better (newer?) cmake idioms -- a good change!

Take what I say with a grain of salt though as I am far from being cmake-fluent.

@jp-dark jp-dark marked this pull request as ready for review June 28, 2024 00:05
Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

👍

As noted on the other PR thread, the ❌ on R's CI run is unrelated.

@jp-dark jp-dark merged commit 8b73a7e into main Jun 28, 2024
34 of 35 checks passed
@jp-dark jp-dark deleted the dark/sc-48622 branch June 28, 2024 13:11
github-actions bot pushed a commit that referenced this pull request Jul 4, 2024
* Add options explicitly to targets instead of to everything. Note: this means compile options are no longer propagated to third-party libraries included with `FetchContent` and `ExternalProject_Add`.
* Move `find_package` calls and coverage flags to base `CMakeLists.txt` instead of adding them twice.
* Re-arrange some CMake calls to reduce redundancy and improve readability.
* Remove unused GIT_COMMIT_HASH compiler definition.
* Change TileDB deprecation flag to use Core flag that is not over-written.
johnkerl pushed a commit that referenced this pull request Jul 4, 2024
* Add options explicitly to targets instead of to everything. Note: this means compile options are no longer propagated to third-party libraries included with `FetchContent` and `ExternalProject_Add`.
* Move `find_package` calls and coverage flags to base `CMakeLists.txt` instead of adding them twice.
* Re-arrange some CMake calls to reduce redundancy and improve readability.
* Remove unused GIT_COMMIT_HASH compiler definition.
* Change TileDB deprecation flag to use Core flag that is not over-written.

Co-authored-by: Julia Dark <24235303+jp-dark@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[c++] Update CMake to use target-based properties
3 participants