introduce single CMake script build all packages #129
Conversation
|
Build passed on following environments:
Build failed on following environments:
These two are outdated releases, does not have newer packages for build dependencies. |
|
@syuu1228 CI is failing because |
syuu1228
force-pushed
the
build_all_packages_in_single_cmake
branch
3 times, most recently
from
64d3280 to
67f1bd9
Compare
|
Waiting with review until CI stops failing. |
syuu1228
force-pushed
the
build_all_packages_in_single_cmake
branch
5 times, most recently
from
5af6a7f to
a92c992
Compare
|
Rewrited almost all code, now CI can pass.
|
Fixed the build failure, now build passed on these distros. |
|
Afaik apart from using prebuiilt libraries (.so + .h) it's popular in C/C++ to use submodules (or something similar) and CMake's Is such usage still possible after your changes? I didn't review the changes yet, but I see fragments: or I can't say I know CMake that well, so I'm not sure: is this going to be a problem? |
|
What's the status of this PR? |
|
@Lorak-mmk can you please review this one? |
I'd prefer to get answer to my question first, because it may affect the PR |
I'm not good at CMake so maybe I'm missing something, but I guess there is no reason to prevent such usage. Note that following APIs causes And it's not because of my patch, it caused on master branch as well.
I did not added any of them, it exist on cpp-rust-driver and also cpp-driver. (BTW maybe I can move back some of the parameters to the if condition, since I initially trying to link libscylla-cpp-driver.so in CMake so I did some compiling/linking in CMake, but I stopped it now) |
There was a problem hiding this comment.
I left few minor comments, apart from that LGTM - although I can't say that I reviewed the changes very carefully, cmake and build / packaging code is usually unreadable black magic to me.
I did verify that the project builds after the changes and that build directory contains all the libraries.
One advantage of this PR not mentioned in description is that now output from cargo is visible during cmake --build build. That's a nice QoL improvement, thank you!
.github/workflows/build.yml
Outdated
| @@ -23,7 +23,7 @@ jobs: | |||
| - name: Setup environment | |||
| run: | | |||
| sudo apt-get update | |||
| sudo apt-get install libssl1.1 libuv1-dev libkrb5-dev libc6-dbg | |||
| sudo apt-get install libssl-dev libuv1-dev libkrb5-dev pkg-config openssl ca-certificates clang cmake cargo libc6-dbg | |||
There was a problem hiding this comment.
According to https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md libssl-dev, pkg-config, openssl, clang, cmake and cargo should be installed by default.
Why is there a need to install them manually?
There was a problem hiding this comment.
Ah, I did not realized most of the dependencies are preinstalled on Github Action instance.
Seems like we don't need to change dependencies at all, I reverted .github/workflows/*.yml changes.
.github/workflows/cassandra.yml
Outdated
| sudo apt-get install libssl1.1 libuv1-dev libkrb5-dev libc6-dbg | ||
| sudo apt-get install libssl-dev libuv1-dev libkrb5-dev pkg-config openssl ca-certificates clang cmake cargo libc6-dbg |
There was a problem hiding this comment.
Reverted this change too.
packaging/scylla-cpp-driver.pc.in
Outdated
There was a problem hiding this comment.
What is the difference between dist and packaging directories?
There was a problem hiding this comment.
On previous version of this patch, I moved pkgconfig files to packaging/, since cpp-driver's CMakeLists.txt will install pkgconfig from the directory so we can reuse cpp-driver's CMake script fewer changes.
But I decided changing this, since packaging/ can be confusing with dist/.
On latest push, I deleted packaging/ and moved pkgconfig files to scylla-rust-wrapper/.
Because pkgconfig will be install in scylla-rust-wrapper/CMakeLists.txt.
Import CMakeRust which add support build Cargo project in CMake: https://github.com/Devolutions/CMakeRust
Seems like this package is not necessary for building cpp-rust-driver, dropping.
syuu1228
force-pushed
the
build_all_packages_in_single_cmake
branch
from
a92c992 to
29ac4ce
Compare
This introduces single CMake script to build all packages and non-package build with same result, including shared library SONAME and versioning. Complicated shellscripting to build package build are no longer required, everything can be build with "cmake -S . -B build && cmake --build build".
syuu1228
force-pushed
the
build_all_packages_in_single_cmake
branch
from
29ac4ce to
493a379
Compare
|
@Lorak-mmk do you think we should wait for @wprzytula / @muzarski review or we can merge? |
With my scarce knowledge of CMake, I doubt that my review would be beneficial in any way. |
This introduces single CMake script to build all packages and non-package build with same result, including shared library SONAME and versioning.
Complicated shellscripting to build package build are no longer required, everything can be build with "cmake -S . -B build && cmake --build build".
Note that there is a limitation, this requires a dummy C code to call libscylla-cpp-driver function.
It is because this rebuild libraries with C Linker on CMake. The dummy C code is located at dummy/dummy.c.
CMakeList.txt is now fully re-written without unrelated codes. And to build Rust code easier, imported CMakeRust project: https://github.com/Devolutions/CMakeRust
And also FindLibClang.cmake from cpp2py:
https://github.com/TRIQS/cpp2py/blob/master/cmake/FindLibClang.cmake
Both included few patches since little bit outdated, and have few bugs.
Pre-review checklist
.github/workflows/build.ymlingtest_filter..github/workflows/cassandra.ymlingtest_filter.