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.

Sign up for GitHub

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

Jump to bottom

use find_packages in cmake files to for shared lib build #1084

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

use find_packages in cmake files to for shared lib build #1084

wants to merge 12 commits into from
+16 −12

Conversation

yulin-li
Copy link
Contributor

@yulin-li yulin-li commented Jan 13, 2023 •
edited
Loading

No description provided.

@lalitb
Copy link
Contributor

lalitb commented Jan 20, 2023 •
edited
Loading

@yulin-li The build script already supports building shared library for 1ds as here.

So, something like this should work:

$ export CMAKE_OPTS='-DBUILD_SHARED_LIBS=ON'
$ ./build-tests.sh release

@yulin-li
Copy link
Contributor Author

@yulin-li The build script already supports building shared library for 1ds as here.

So, something like this should work:

$ export CMAKE_OPTS='-DBUILD_SHARED_LIBS=ON'
$ ./build-tests.sh release

oh thanks, let me revert the changes in build-test.sh. I think it's still worth to keep the find_package updates.

@yulin-li yulin-li changed the title update scripts/cmake files to imporve shared lib build propcess use find_packages in cmake files to for shared lib build Jan 21, 2023
@lalitb
Copy link
Contributor

lalitb commented Jan 23, 2023 •
edited
Loading

oh thanks, let me revert the changes in build-test.sh. I think it's still worth to keep the find_package updates.

This would be a problem if we install sqlite3 from (say) apt package manager. As "sqlite3" or "sqlite3-dev" package installed through it doesn't seem to include a CMake package config file. Do you see issue without find_package, as the build will eventually fail anyway if sqlite3 is not installed ?

# dpkg-query -L  libsqlite3-dev:amd64
/.
/usr
/usr/include
/usr/include/sqlite3.h
/usr/include/sqlite3ext.h
/usr/lib
/usr/lib/x86_64-linux-gnu
/usr/lib/x86_64-linux-gnu/libsqlite3.a
/usr/lib/x86_64-linux-gnu/libsqlite3.la
/usr/lib/x86_64-linux-gnu/pkgconfig
/usr/lib/x86_64-linux-gnu/pkgconfig/sqlite3.pc
/usr/share
/usr/share/doc
/usr/share/doc/libsqlite3-dev
/usr/share/doc/libsqlite3-dev/copyright
/usr/lib/x86_64-linux-gnu/libsqlite3.so
/usr/share/doc/libsqlite3-dev/changelog.Debian.gz

# dpkg-query -L   libsqlite3-0:amd64
/.
/usr
/usr/lib
/usr/lib/x86_64-linux-gnu
/usr/lib/x86_64-linux-gnu/libsqlite3.so.0.8.6
/usr/share
/usr/share/doc
/usr/share/doc/libsqlite3-0
/usr/share/doc/libsqlite3-0/README.Debian
/usr/share/doc/libsqlite3-0/changelog.Debian.gz
/usr/share/doc/libsqlite3-0/copyright
/usr/lib/x86_64-linux-gnu/libsqlite3.so.0

# dpkg-query -L sqlite3
/.
/usr
/usr/bin
/usr/bin/sqldiff
/usr/bin/sqlite3
/usr/share
/usr/share/doc
/usr/share/doc/sqlite3
/usr/share/doc/sqlite3/copyright
/usr/share/man
/usr/share/man/man1
/usr/share/man/man1/sqldiff.1.gz
/usr/share/man/man1/sqlite3.1.gz
/usr/share/doc/sqlite3/changelog.Debian.gz

@lalitb
Copy link
Contributor

lalitb commented Jan 31, 2023

@yulin-li - let us know if the PR is still valid ?

@yulin-li yulin-li requested a review from a team as a code owner March 3, 2024 05:23
@yulin-li
Copy link
Contributor Author

yulin-li commented Mar 3, 2024 

CMAKE_OPTS='-DBUILD_SHARED_LIBS=ON'

sorry for this very late reply. The camke configurations is actually provided by cmake (https://github.com/Kitware/CMake/blob/master/Modules/FindSQLite3.cmake), not sqlite3 package.

The problem without find_packages is, we cannot use sqlite3 installed by package managers, we need to install a duplicated one in /usr/local/lib/

@yulin-li
Copy link
Contributor Author

yulin-li commented Mar 3, 2024

**lalitb ** c

@lalitb this PR is still valid, which allows us to use sqlite3 libs installed by package managers.

@yulin-li
Copy link
Contributor Author

@ThomsonTan I saw you were updating this PR. Could it be reviewed and merge? So we can avoid patching it in our project.

@ThomsonTan
Copy link
Contributor

@yulin-li, is this PR still valid? Does the consuming of local install sqlite3 break other users who may not epect the local install?

@yulin-li
Copy link
Contributor Author

@yulin-li, is this PR still valid? Does the consuming of local install sqlite3 break other users who may not epect the local install?

This is still valid. The local sqlite is only required if the user wants to build a shared lib. This won't change the behavior for static lib build.

ThomsonTan
ThomsonTan reviewed Sep 24, 2024
View reviewed changes
lib/CMakeLists.txt
@@ -276,7 +276,9 @@ if(BUILD_SHARED_LIBS STREQUAL "ON")
# Prefer shared libraries for sqlite3 and zlib
add_library(sqlite3 SHARED IMPORTED GLOBAL)
add_library(z SHARED IMPORTED GLOBAL)
target_link_libraries(mat PUBLIC sqlite3 PUBLIC z ${LIBS} "${CMAKE_THREAD_LIBS_INIT}" "${CMAKE_DL_LIBS}" "${CMAKE_REQUIRED_LIBRARIES}")
find_package(SQLite3 REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

find_package(SQLite3 REQUIRED)

Is there any scenario where the call to find_package(SQLite3 REQUIRED) is required? Assume the current hard-coded name still works, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the current hard-coded name still works. But I think the find_package is a preferred way by cmake, and it will report a clearer error if sqlite3 is not installed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomsonTan ThomsonTan ThomsonTan left review comments

@kindbe kindbe Awaiting requested review from kindbe

@maxgolov maxgolov Awaiting requested review from maxgolov

@mkoscumb mkoscumb Awaiting requested review from mkoscumb

@reyang reyang Awaiting requested review from reyang

@bliptec bliptec Awaiting requested review from bliptec

@lalitb lalitb Awaiting requested review from lalitb

@sid-dahiya sid-dahiya Awaiting requested review from sid-dahiya

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yulin-li @lalitb @ThomsonTan