Skip to content

Add install capabilities to top-level CMake config #101

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

Closed
wants to merge 3 commits into from

Conversation

avelanarius
Copy link

@avelanarius avelanarius commented Jan 17, 2023

Before this change, the generated Makefile by CMake lacked the capability to run make install. This PR makes it possible
to run make install, for example:

mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=RELEASE -DCASS_INSTALL_PKG_CONFIG=OFF -DCMAKE_INSTALL_PREFIX:PATH=/usr -DCMAKE_INSTALL_LIBDIR=/usr/lib64 ..
make DESTDIR=/tmp/example install

The names and contents of installed files try to mimic the original C++ Driver files. For example, installation of C++ Driver generates the following .so files/symlinks:

usr/lib64/libscylla-cpp-driver.so -> libscylla-cpp-driver.so.2
usr/lib64/libscylla-cpp-driver.so.2 -> libscylla-cpp-driver.so.2.16.2
usr/lib64/libscylla-cpp-driver.so.2.16.2

This particular behavior is replicated in a new build-cmake-compat.sh script, which is executed as a BUILD_COMMAND in the Rust external project in the top-level CMakeLists.txt.

Unfortunately, I have not been able to configure CMake to create such symlinks on its own. One of the reasons is an unfixed bug in CMake: https://gitlab.kitware.com/cmake/cmake/-/issues/23249#note_1277271. Therefore, the build-cmake-compat.sh is introduced to create the necessary files in a way it's easy to install for CMake.

The third commit includes the static library in the installed files.

Those are the contents of DESTDIR (as in make DESTDIR=... install) in the original C++ driver:

DESTDIR
└── usr
    ├── include
    │   ├── cassandra.h
    │   └── dse.h
    └── lib64
        ├── libscylla-cpp-driver.so -> libscylla-cpp-driver.so.2
        ├── libscylla-cpp-driver.so.2 -> libscylla-cpp-driver.so.2.16.2
        ├── libscylla-cpp-driver.so.2.16.2
        └── libscylla-cpp-driver_static.a

and after this PR in the bindings:

DESTDIR
└── usr
    ├── include
    │   └── cassandra.h
    └── lib64
        ├── libscylla-cpp-driver.so -> libscylla-cpp-driver.so.0
        ├── libscylla-cpp-driver.so.0 -> libscylla-cpp-driver.so.0.0.1
        ├── libscylla-cpp-driver.so.0.0.1
        └── libscylla-cpp-driver_static.a

This PR is a prerequisite for a follow-up PR reworking #88.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

Copy a part of CMakeLists.txt from the original C++ Driver 
CMakeLists.txt. The copied code snippet sets the CMAKE_INSTALL_LIBDIR
variable if it wasn't specified explicitly. This commit is required
by a follow-up commit.
Before this change, the generated Makefile by CMake lacked the 
capability to run "make install". This patch makes it possible
to run "make install", for example:

mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=RELEASE -DCASS_INSTALL_PKG_CONFIG=OFF -DCMAKE_INSTALL_PREFIX:PATH=/usr -DCMAKE_INSTALL_LIBDIR=/usr/lib64 ..
make DESTDIR=/tmp/example install

The names and contents of installed files try to mimic the original
C++ Driver files. For example, installation of C++ Driver generates the 
following .so files/symlinks:

usr/lib64/libscylla-cpp-driver.so -> libscylla-cpp-driver.so.2
usr/lib64/libscylla-cpp-driver.so.2 -> libscylla-cpp-driver.so.2.16.2
usr/lib64/libscylla-cpp-driver.so.2.16.2

This particular behavior is replicated in a new build-cmake-compat.sh
script, which is executed as a BUILD_COMMAND in the Rust external 
project in the top-level CMakeLists.txt.

Unfortunately, I have not been able to configure CMake to create
such symlinks on its own. One of the reasons is an unfixed bug in CMake:
https://gitlab.kitware.com/cmake/cmake/-/issues/23249#note_1277271.
Therefore, the build-cmake-compat.sh is introduced to create the
necessary files in a way it's easy to install for CMake.
Add copying of .a static library file to build-cmake-compat.sh file
when CASS_BUILD_STATIC is ON. The copied file name mimics the filename
of the original C++ driver.

After this commit, the contents of DESTDIR (as in make DESTDIR=... install)
are the same between the C++-Rust bindings and the original C++ driver
(apart from the usr/include/dse.h file).
Copy link
Contributor

@Gor027 Gor027 left a comment

Choose a reason for hiding this comment

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

I manually enabled the flag CASS_BUILD_STATIC in CMakeLists.txt, however, after the build finished .a file was not created in the destination directory. I built it with the same commands as in the PR description. Should I build it in another way?

Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

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

I've tried it out and the results were very good. The -DCASS_BUILD_STATIC option worked as well, respecting correctly options: =false, =OFF as non-static build and =true, =ON as static. The .a file was present in the target dir.

Comment on lines +4 to +6
# The version number should be in the third line
# of Cargo.toml file (e.g. version = "0.0.1")
VERSION_NUMBER=$(sed -n '3p' < Cargo.toml)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this an assumption that is prone to change?

create_symlinks() {
rm -rf target/$1/cmake-compat/
mkdir target/$1/cmake-compat/

cp target/$1/libscylla_cpp_driver.so target/$1/cmake-compat/libscylla-cpp-driver.so.$MAJOR.$MINOR.$PATCH

if [[ "$COPY_STATIC" == true ]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is comparison to true necessary in Bash??

@roydahan
Copy link
Collaborator

roydahan commented May 9, 2024

@avelanarius is this one still needed?
If so, what's preventing it from being merged?

@roydahan
Copy link
Collaborator

roydahan commented Jun 3, 2024

Closing in favor of #88

@roydahan roydahan closed this Jun 3, 2024
@Lorak-mmk
Copy link
Collaborator

Did you mean #122 ?

@roydahan
Copy link
Collaborator

roydahan commented Jun 3, 2024

Yes

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.

5 participants