Support rpm and deb packaging

syuu1228 commented 2 months ago

Add build_rpm.sh and build_deb.sh build scripts to produce rpm & deb package.

closes scylladb/scylla-pkg#3714

closes #121

piodul commented 2 months ago

Just to be clear, the failing clippy check has nothing to do with this change - we are using the newest version of the clippy linter and apparently new lints were introduced which our code does not satisfy.

It would probably be nice to fix them first to have green CI, but I personally don't think it's required.

Opened an issue about it: https://github.com/scylladb/cpp-rust-driver/issues/123

wprzytula commented 2 months ago

I've successfully packaged the driver into RPM and installed it on my Fedora 39. Congratulations, @syuu1228! :rocket:

roydahan commented 2 months ago

@syuu1228 where are we standing with this one? Can you please address/reply the comments?

syuu1228 commented 2 months ago

Fixed codes mentioned on comments, also bash coding style fixed (by following ShellCheck rules)

syuu1228 commented 2 months ago

Just realized that generating pkgconfig files on deb was not implemented yet, fixed.

roydahan commented 1 month ago

We would really want to merge and release 0.1.0 by the end of next week. So, whoever should review/re-review and/or address comments, please prioritze.

wprzytula commented 1 month ago

One more thing: apart from the commit with packaging there are 2 more commits. First one claims to update Rust Driver to newest version, but updates it to a commit from August of last year. Unless those 2 commits are necessary (and I don't see why would they be) please stick to the main goal of this PR which is packaging and drop those 2 commits. We'll update Rust Driver in a separate PR.

@Lorak-mmk These two commits are both mine, and they are already merged to master; I have no idea why they are displayed here.

Lorak-mmk commented 1 month ago

One more thing: apart from the commit with packaging there are 2 more commits. First one claims to update Rust Driver to newest version, but updates it to a commit from August of last year. Unless those 2 commits are necessary (and I don't see why would they be) please stick to the main goal of this PR which is packaging and drop those 2 commits. We'll update Rust Driver in a separate PR.

@Lorak-mmk These two commits are both mine, and they are already merged to master; I have no idea why they are displayed here.

Rebasing a PR on master will probably fix this

wprzytula commented 1 month ago

cpp-rust-driver has dashes in .so name while cpp-driver has underscores. Is it going to be a problem for a user looking for a drop-in replacement?

Good catch! That's probably a very important question.

syuu1228 commented 1 month ago

What is the .build-id folder?

It is the standard place to store debuginfo on newer distribution. Both RedHat variants and Debian variants use same place. reference: https://fedoraproject.org/wiki/RolandMcGrath/BuildID

2. Why do we only have .so without version number in the name? cpp-driver provides, as you can see, .so.<major-version> and .so.<full-version>, which are symlinks to main .so. Why is that?

I implemented workaround patch for this: https://github.com/scylladb/cpp-rust-driver/compare/5036de631e6271d15d419a327210b05df2ce418d..f552e9c62cc0d45ed074414a3d3f8aa69b1aabbf

On C/C++ project, build system automatically generate these things, but cdylib in Cargo does not, so we need to take care manually. Not just about so.X and so.X.Y symlinks, cdylib in Cargo also does not have SONAME support, so I added workaround for this too. thread: https://github.com/scylladb/cpp-rust-driver/pull/122#discussion_r1600976090

3. cpp-rust-driver has dashes in .so name while cpp-driver has underscores. Is it going to be a problem for a user looking for a drop-in replacement?

I think both filename and SONAME should be identical as latest cpp-driver (libscylla-cpp-driver.so.2 -> libscylla-cpp-driver.so.2.16.2).

To get maximum compatibility with cpp-driver, maybe we should try librsvg approach (described above). If we can use cpp-drvier's CMakeLists.txt to link a shared library and install it, I guess we will get very similar output than original one. And probably it can be implement by replacing C++ targets with ExternalProject_Add() which invoke cargo build to generate staticlib.

syuu1228 commented 1 month ago

3. cpp-rust-driver has dashes in .so name while cpp-driver has underscores. Is it going to be a problem for a user looking for a drop-in replacement?

Fixed this issue on versioning.sh. New file name becomes like this:

$ ls -l /usr/lib64/libscylla*
-rw-r--r-- 1 root root 7746332 Mar 28 09:00 /usr/lib64/libscylla_cpp_driver.a
lrwxrwxrwx 1 root root      30 Mar 28 09:00 /usr/lib64/libscylla-cpp-driver.so -> libscylla-cpp-driver.so.2.16.1
lrwxrwxrwx 1 root root      30 Mar 28 09:00 /usr/lib64/libscylla-cpp-driver.so.2 -> libscylla-cpp-driver.so.2.16.1
-rwxr-xr-x 1 root root 3193400 Mar 28 09:00 /usr/lib64/libscylla-cpp-driver.so.2.16.1

Note that I tried to rename this in Cargo.toml, but I find out it can't since Cargo reject that. It says:

$ cargo build
error: failed to parse manifest at `/home/syuu/cpp-rust-driver.3/scylla-rust-wrapper/Cargo.toml`

Caused by:
  library target names cannot contain hyphens: scylla-cpp-driver

wprzytula commented 1 month ago

The CI is failing due to new problems being detected by a new toolchain. #124 fixes them, so rebasing on it (once it gets merged) should make the CI pass.

wprzytula commented 1 month ago

The CI is failing due to new problems being detected by a new toolchain. #124 fixes them, so rebasing on it (once it gets merged) should make the CI pass.

Merged. Please rebase on master @syuu1228.

syuu1228 commented 1 month ago

Rebased.

yaronkaikov commented 1 month ago

@Lorak-mmk Please review again

Lorak-mmk commented 1 month ago

cpp-rust-driver has dashes in .so name while cpp-driver has underscores. Is it going to be a problem for a user looking for a drop-in replacement?

Fixed this issue on versioning.sh. New file name becomes like this:
$ ls -l /usr/lib64/libscylla*
-rw-r--r-- 1 root root 7746332 Mar 28 09:00 /usr/lib64/libscylla_cpp_driver.a
lrwxrwxrwx 1 root root      30 Mar 28 09:00 /usr/lib64/libscylla-cpp-driver.so -> libscylla-cpp-driver.so.2.16.1
lrwxrwxrwx 1 root root      30 Mar 28 09:00 /usr/lib64/libscylla-cpp-driver.so.2 -> libscylla-cpp-driver.so.2.16.1
-rwxr-xr-x 1 root root 3193400 Mar 28 09:00 /usr/lib64/libscylla-cpp-driver.so.2.16.1
Note that I tried to rename this in Cargo.toml, but I find out it can't since Cargo reject that. It says:
$ cargo build
error: failed to parse manifest at `/home/syuu/cpp-rust-driver.3/scylla-rust-wrapper/Cargo.toml`

Caused by:
  library target names cannot contain hyphens: scylla-cpp-driver

You said you fixed the issue but the snippet you posted still has dashes instead of underscores - why? I see dashes in versioning.sh and in the RPM I've built. I also don't see the .so without the version. List of the files in the rpm:


/usr/lib/.build-id
/usr/lib/.build-id/51
/usr/lib/.build-id/51/dfc674c62e991c55ee5ea8fec011a8fe8d9ac3
/usr/lib64/libscylla-cpp-driver.so.2
/usr/lib64/libscylla-cpp-driver.so.2.16.1
/usr/share/doc/scylla-cpp-rust-driver
/usr/share/doc/scylla-cpp-rust-driver/LICENSE
/usr/share/doc/scylla-cpp-rust-driver/README.md
``

syuu1228 commented 1 month ago

You said you fixed the issue but the snippet you posted still has dashes instead of underscores - why?

I think it's opposite, it should be replace underscores with dashes, since we want to rename libscylla_cpp_driver.so.X.Y.Z to libscylla-cpp-driver.so.X.Y.Z. But I found that I did not renamed static library yet, so I implemented it and pushed new version.

The new version also contains follow changes:

We did not changed static library filename (libscylla_cp_driver.a). Now it renamed to same as cpp-driver (libscylla-cpp-driver_static.a)
pkg-config file are broken, since we renamed library name. Applied dashes style name (libscylla-cpp-driver)
Change include path to original one, for compatibility (/usr/include)
Add Conflicts with cpp-driver rpm/deb package
Rebased with master

syuu1228 commented 1 month ago

I found that versioning.sh will causes mv: 'target/release/libscylla_cpp_driver.a' and 'target/release/libscylla-cpp-driver_static.a' are the same file error when we build twice on same profile. This is not occur on build_rpm.sh and build_deb.sh since both are running in temporary container, but if we build the library manually or calling versioning.sh from CMake will be problematic.

To avoid the error, stop calling mv and use cp --remove-destination to remove the destination file first. Note: ln is already safe since -f option does same behavior (remove the destination file first).

Lorak-mmk commented 1 month ago

You said you fixed the issue but the snippet you posted still has dashes instead of underscores - why?

I think it's opposite, it should be replace underscores with dashes, since we want to rename libscylla_cpp_driver.so.X.Y.Z to libscylla-cpp-driver.so.X.Y.Z. But I found that I did not renamed static library yet, so I implemented it and pushed new version.

Oh, that's right. In my initial comment about it I posted correct file lists and then mistakenly wrote "cpp-rust-driver has dashes in .so name while cpp-driver has underscores" while it was the other way around. After bulding the package I see those files inside:

/usr/lib/.build-id
/usr/lib/.build-id/61
/usr/lib/.build-id/61/48a6727fa1f90104938a95ad2845314910309a
/usr/lib64/libscylla-cpp-driver.so.2
/usr/lib64/libscylla-cpp-driver.so.2.16.1
/usr/share/doc/scylla-cpp-rust-driver
/usr/share/doc/scylla-cpp-rust-driver/LICENSE
/usr/share/doc/scylla-cpp-rust-driver/README.md

/usr/lib64/libscylla-cpp-driver.so is still missing compared to cpp-driver.

Another small issue: after building the package with ./dist/redhat/build_rpm.sh --target fedora-39-x86_64 a version file is created in main folder of repo. I don't fully understand why, I thought that the build was done in some sort of container. Anyways, it should be either removed or added to .gitignore.

What is the .build-id folder?

It is the standard place to store debuginfo on newer distribution. Both RedHat variants and Debian variants use same place.

I don't see this folder in .deb packages. Is this expected? Oh wait, I actually see it in dbgsym deb, I assume this is expected.

Another question, hopefully last one: In dev packages (both rpm and deb) I don't see versioned SOs (.so.2, .so.2.16.1), only the normal one (.so). Is this expected?

syuu1228 commented 1 month ago

Add a workaround patch for Ubuntu 24.04 build, since Ubuntu 24.04 version of patchelf breaks libscylla-cpp-driver.so for some reason (probably a bug on patchelf). It will cause error when stripping debug symbol since the ELF header is corrupted.

The error message is like this:

        readelf -W --section-headers debian/libscylla-cpp-driver-dev/usr/lib/x86_64-linux-gnu/libscylla-cpp-driver_static.a | sed -n '/^ *\[[ 0-9]*]/s/ *\[[ 0-9]*\] *//p' | awk 'BEGIN {rv=1} /^NULL/ {next} $1 ~ /^.(text|data|(preinit|init|fini)_array$)/ {if ($5 !~ /^0+$/) rv=0} END { exit rv}'
f132a9c6e662bd19d130b1e3ee434af824abbfe0
readelf: Error: no .dynamic section in the dynamic segment
        install -m0755 -d debian/.debhelper/libscylla-cpp-driver0/dbgsym-root/usr/lib/debug/.build-id/f1
        objcopy --only-keep-debug --compress-debug-sections debian/libscylla-cpp-driver0/usr/lib/x86_64-linux-gnu/libscylla-cpp-driver.so.2.16.1 debian/.debhelper/libscylla-cpp-driver0/dbgsym-root/usr/lib/debug/.build-id/f1/32a9c6e662bd19d130b1e3ee434af824abbfe0.debug
objcopy: warning: debian/libscylla-cpp-driver0/usr/lib/x86_64-linux-gnu/libscylla-cpp-driver.so.2.16.1 has a corrupt string table index
objcopy: unable to modify 'debian/libscylla-cpp-driver0/usr/lib/x86_64-linux-gnu/libscylla-cpp-driver.so.2.16.1' due to errors
dh_strip: error: objcopy --only-keep-debug --compress-debug-sections debian/libscylla-cpp-driver0/usr/lib/x86_64-linux-gnu/libscylla-cpp-driver.so.2.16.1 debian/.debhelper/libscylla-cpp-driver0/dbgsym-root/usr/lib/debug/.build-id/f1/32a9c6e662bd19d130b1e3ee434af824abbfe0.debug returned exit code 1
dh_strip: error: Aborting due to earlier error
make: *** [debian/rules:15: binary] Error 25
dpkg-buildpackage: error: fakeroot debian/rules binary subprocess returned exit status 2
debuild: fatal error at line 1184:
dpkg-buildpackage -us -uc -ui failed

This can be avoid by using Ubuntu 23.10's patchelf, the patch do that in versioning.sh. (This should be temporary solution but I not find other way for now)

syuu1228 commented 1 month ago

/usr/lib64/libscylla-cpp-driver.so is still missing compared to cpp-driver.

Sorry I forgot to describe what is this. On both RedHat and Debian, library packages does not include libXXX.so file on main package, it will installed in -devel (or -dev in Debian) package.

Debian Policy Manual says: The development package should contain a symlink for the associated shared library without a version number. For example, the libgdbm-dev package should include a symlink from /usr/lib/libgdbm.so to libgdbm.so.3.0.0. This symlink is needed by the linker (ld) when compiling packages, as it will only look for libgdbm.so when compiling dynamically. https://www.debian.org/doc/debian-policy/ch-sharedlibs.html#development-files

Seems like cpp-driver packaging libscylla-cpp-driver.so in main package, but unless the difference causes compatibility problem, I think we should keep current packaging since it looks correct.

syuu1228 commented 1 month ago

Another question, hopefully last one: In dev packages (both rpm and deb) I don't see versioned SOs (.so.2, .so.2.16.1), only the normal one (.so). Is this expected?

I think this is same topic as I described above, -dev package will contain libXXX.so, and main package will contain libXXX.so.1.2.3 and libXXX.so.1.

Lorak-mmk commented 1 month ago

Ok, so the cpp-driver is in the wrong here IIUC. If so, please fix the last tiny issue I mentioned (version file, which should be .gitignore'd / removed after build / not created) and we can merge.

syuu1228 commented 1 month ago

I just found that we actually can fix SONAME using RUSTFLAGS, even we build Rust in Cargo: https://developers.redhat.com/articles/2022/09/05/how-create-c-binding-rust-library#fixing_the_soname I think this is correct approach than paching ELF header, and it also should fix Ubuntu 24.04 patchelf issue, so pushed new version to do that.

syuu1228 commented 1 month ago

If so, please fix the last tiny issue I mentioned (version file, which should be .gitignore'd / removed after build / not created) and we can merge.

Pushed new version to add version to .gitignore.

syuu1228 commented 1 month ago

@Lorak-mmk Pushed new version to change RUSTFLAGS on both rpm & deb.

roydahan commented 1 month ago

@syuu1228 can you please check why CI fails?

syuu1228 commented 1 month ago

@roydahan This is a bug on .github/workflows, sent fix at #127

roydahan commented 1 month ago

@wprzytula would you want the honor to merge? Once we merge, we need to officially build and release 0.1.0.

wprzytula commented 1 month ago

@wprzytula would you want the honor to merge? Once we merge, we need to officially build and release 0.1.0.

Gladly!

scylladb / cpp-rust-driver

Support rpm and deb packaging #122