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

Use CMake's SQLite targets #58726

Merged
merged 1 commit into from
Sep 15, 2024
Merged

Use CMake's SQLite targets #58726

merged 1 commit into from
Sep 15, 2024

Conversation

Simon-Lopez
Copy link
Contributor

The ./cmake/FindSqlite3.cmake is modified to use the same targets and variables introduced in the FindSQLite3 module in CMake starting with version 3.14.

The other CMakeFiles.txt are modified accordingly.

Fixes #56885

@github-actions github-actions bot added this to the 3.40.0 milestone Sep 13, 2024
@@ -20,12 +20,12 @@ include_directories(
${CMAKE_BINARY_DIR}
)
include_directories(SYSTEM
${SQLITE3_INCLUDE_DIR}
${SQLite3_INCLUDE_DIRS}
Copy link
Contributor

Choose a reason for hiding this comment

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

include_directories() should not longer be needed because the below target_link_libraries() now uses the SQLite::SQLite3 target name, which declares INTERFACE_INCLUDE_DIRECTORIES per your above changes (cf https://schneide.blog/2016/04/08/modern-cmake-with-target_link_libraries/)

Copy link
Contributor Author

@Simon-Lopez Simon-Lopez Sep 13, 2024

Choose a reason for hiding this comment

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

Good point! I'll make the changes. Thanks.

Copy link

github-actions bot commented Sep 13, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit c06c7c9)

)

target_link_libraries(qgis_bench
qgis_core
${SQLITE3_LIBRARY}
SQLite::SQLite3
Copy link
Member

Choose a reason for hiding this comment

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

This is also inherited from qgis_core, you can remove this line too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedbacks.
Should be ok now.

CMakeLists.txt Outdated
# This bypasses the FindSQLite3 module introduced in CMake 3.14
# On case insensitive platforms (e.g. Windows) this is because
# ./cmake/FindSqlite3.cmake comes first on the CMAKE_MODULE_PATH
# (otherwise it is because of the case: *Sqlite3* vs. *SQLite3*)
find_package(Sqlite3)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should just make it REQUIRED here and kill the checks below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine for me I just kept it like it was before. :-)

This uses the same targets and variables introduced in
the FindSQLite3 module in CMake starting with version 3.14.

The other CMakeFiles.txt are modified accordingly.
@m-kuhn
Copy link
Member

m-kuhn commented Sep 15, 2024

Thanks for this!

@m-kuhn m-kuhn merged commit 7814e12 into qgis:master Sep 15, 2024
29 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.

Configure fails with PROJ version 9.4.0
3 participants