disabling branchwater feature leads to Python test error: symbol 'revindex_free' not found in library … _lowlevel.so: undefined symbol: revindex_free

[mr-c]

Hello,

While preparing a Debian package for Sourmash, we are getting the follow error:

==================================== ERRORS ====================================
_____________________ ERROR collecting tests/test_index.py _____________________
tests/test_index.py:23: in <module>
    from sourmash.index.revindex import RevIndex
sourmash/index/revindex.py:14: in <module>
    class RevIndex(RustObject, Index):
sourmash/index/revindex.py:15: in RevIndex
    __dealloc_func__ = lib.revindex_free
E   ffi.error: symbol 'revindex_free' not found in library '/build/sourmash-4.8.11/.pybuild/cpython3_3.12/build/sourmash/_lowlevel/_lowlevel.so': /build/sourmash-4.8.11/.pybuild/cpython3_3.12/build/sourmash/_lowlevel/_lowlevel.so: undefined symbol: revindex_free

The full log is at https://salsa.debian.org/med-team/sourmash/-/jobs/6498975

Here are the patches we had to apply to adjust the dependencies to match the version of Rust crates currently available in Debian's development version (sid a.k.a. unstable): https://salsa.debian.org/med-team/sourmash/-/tree/master/debian/patches?ref_type=heads

Here is a Dockerfile to reproduce the issue:

FROM docker.io/debian:sid-slim

WORKDIR /build

RUN apt-get update && apt-get install -y --no-install-recommends git ca-certificates && apt-get upgrade -y
RUN git clone --branch with_RevIndex_tests https://salsa.debian.org/med-team/sourmash.git
WORKDIR /build/sourmash
RUN apt-get -y build-dep .
RUN dpkg-buildpackage -uc -us -b

[ctb]

it's here:

sourmash/src/core/src/ffi/index/revindex.rs

Lines 134 to 137 in 6ae9cd3

	#[no_mangle]
	pub unsafe extern "C" fn revindex_free(ptr: *mut SourmashRevIndex) {
	SourmashRevIndex::drop(ptr);
	}

and it looks like #[no_mangle] might be the problem? ref: https://internals.rust-lang.org/t/precise-semantics-of-no-mangle/4098

but I don't know enough about rustc, linking, and FFI to understand what we should be doing.

[mr-c]

I can open PRs to upgrade some of the dependencies to the newer versions found in Debian. Perhaps that will surface the issue and have positive side-effects.

[luizirber]

Hey Michael!

While preparing a Debian package for Sourmash, we are getting the follow error:

==================================== ERRORS ====================================
_____________________ ERROR collecting tests/test_index.py _____________________
tests/test_index.py:23: in <module>
    from sourmash.index.revindex import RevIndex
sourmash/index/revindex.py:14: in <module>
    class RevIndex(RustObject, Index):
sourmash/index/revindex.py:15: in RevIndex
    __dealloc_func__ = lib.revindex_free
E   ffi.error: symbol 'revindex_free' not found in library '/build/sourmash-4.8.11/.pybuild/cpython3_3.12/build/sourmash/_lowlevel/_lowlevel.so': /build/sourmash-4.8.11/.pybuild/cpython3_3.12/build/sourmash/_lowlevel/_lowlevel.so: undefined symbol: revindex_free

The full log is at https://salsa.debian.org/med-team/sourmash/-/jobs/6498975

include/sourmash.h is built with all features, and your patch series removes the branchwater feature, so it would be better to regen the header. There is a Makefile rule for it, but I think it will complicate the build a lot...

Anything we can do on our side to help with bringing back the branchwater feature for the Debian package?

Here are the patches we had to apply to adjust the dependencies to match the version of Rust crates currently available in Debian's development version (sid a.k.a. unstable): https://salsa.debian.org/med-team/sourmash/-/tree/master/debian/patches?ref_type=heads

On the soften-deps patch the versions are specified as

-camino = { version = "1.1.7", features = ["serde1"] }
+camino = { version = "1.1", features = ["serde1"] }

I think it has to be

camino = { version = "1.1.0", features = ["serde1"] }

to avoid camino 1.2.x being selected. Hopefully that wouldn't break anything semantic versioning-wise, and we can always bump here if needed.

I've been trying to hold the MSRV pretty low to account for Debian not having the latest rustc compiler, I haven't considered how crate versions would interact. I'm assuming the Debian build system ignores Cargo.lock? In this case dependabot PRs here don't break the package build (it only updates Cargo.lock), but we would need to pay more attention when bumping version ranges in Cargo.toml

Here is a Dockerfile to reproduce the issue:

FROM docker.io/debian:sid-slim

WORKDIR /build

RUN apt-get update && apt-get install -y --no-install-recommends git ca-certificates && apt-get upgrade -y
RUN git clone --branch with_RevIndex_tests https://salsa.debian.org/med-team/sourmash.git
WORKDIR /build/sourmash
RUN apt-get -y build-dep .
RUN dpkg-buildpackage -uc -us -b

Oh, cool! Would something close to this be a good CI check on our side to see if the package still builds properly when we update dependencies/code in PRs?

[mr-c]

Hey Michael!

Hey Luiz!

While preparing a Debian package for Sourmash, we are getting the follow error:

==================================== ERRORS ====================================
_____________________ ERROR collecting tests/test_index.py _____________________
tests/test_index.py:23: in <module>
    from sourmash.index.revindex import RevIndex
sourmash/index/revindex.py:14: in <module>
    class RevIndex(RustObject, Index):
sourmash/index/revindex.py:15: in RevIndex
    __dealloc_func__ = lib.revindex_free
E   ffi.error: symbol 'revindex_free' not found in library '/build/sourmash-4.8.11/.pybuild/cpython3_3.12/build/sourmash/_lowlevel/_lowlevel.so': /build/sourmash-4.8.11/.pybuild/cpython3_3.12/build/sourmash/_lowlevel/_lowlevel.so: undefined symbol: revindex_free

The full log is at https://salsa.debian.org/med-team/sourmash/-/jobs/6498975

include/sourmash.h is built with all features, and your patch series removes the branchwater feature, so it would be better to regen the header. There is a Makefile rule for it, but I think it will complicate the build a lot...

ah, revindex_free is from the branchwater feature. Okay, I'll update the patch to remove that function entirely. Thanks for the tip!

Anything we can do on our side to help with bringing back the branchwater feature for the Debian package?

Restoring the branchwater feature in the Debian package of sourmash would require packaging the rocksdb crate for Debian: https://wiki.debian.org/Teams/RustPackaging#Packages

Here are the patches we had to apply to adjust the dependencies to match the version of Rust crates currently available in Debian's development version (sid a.k.a. unstable): https://salsa.debian.org/med-team/sourmash/-/tree/master/debian/patches?ref_type=heads

On the soften-deps patch the versions are specified as

-camino = { version = "1.1.7", features = ["serde1"] }
+camino = { version = "1.1", features = ["serde1"] }

I think it has to be

camino = { version = "1.1.0", features = ["serde1"] }

to avoid camino 1.2.x being selected. Hopefully that wouldn't break anything semantic versioning-wise, and we can always bump here if needed.

Right. Debian only has a single version at a time for each Rust crate, unless we make extra effort. Currently only version 1.1.6 of the camino crate is packaged in Debian unstable:

$ rmadison rust-camino
rust-camino | 1.0.5-1       | stable     | source
rust-camino | 1.1.6-1       | testing    | source
rust-camino | 1.1.6-1       | unstable   | source

I've been trying to hold the MSRV pretty low to account for Debian not having the latest rustc compiler, I haven't considered how crate versions would interact. I'm assuming the Debian build system ignores Cargo.lock? In this case dependabot PRs here don't break the package build (it only updates Cargo.lock), but we would need to pay more attention when bumping version ranges in Cargo.toml

The Debian packaging tools regenerate the Cargo.lock based upon the available crate version after applying our patches, yes.

Here is a Dockerfile to reproduce the issue:

FROM docker.io/debian:sid-slim

WORKDIR /build

RUN apt-get update && apt-get install -y --no-install-recommends git ca-certificates && apt-get upgrade -y
RUN git clone --branch with_RevIndex_tests https://salsa.debian.org/med-team/sourmash.git
WORKDIR /build/sourmash
RUN apt-get -y build-dep .
RUN dpkg-buildpackage -uc -us -b

Oh, cool! Would something close to this be a good CI check on our side to see if the package still builds properly when we update dependencies/code in PRs?

A version of this, yes. The above recipe is based upon the tarball of Sourmash from https://github.com/sourmash-bio/sourmash/releases/tag/v4.8.11 → https://github.com/sourmash-bio/sourmash/archive/refs/tags/v4.8.11.tar.gz

One could adjust the recipe to copy from the container build context into the working directory and then copy the debian directory from https://salsa.debian.org/med-team/sourmash (instead of the RUN git clone line above) and then continuing on with the RUN apt-get -y build-dep, etc..

[ctb]

can we do conditional compilation of the revindex_free fn based on the branchwater feature?

[mr-c]

can we do conditional compilation of the revindex_free fn based on the branchwater feature?

That would be nice for Debian, until we get rocksdb in our archive as well.

@luizirber Your hint worked, we no longer get the error!

https://salsa.debian.org/med-team/sourmash/-/commit/1e81fd744ffd4e764bbb25648417b9da0701e060#371356d4e7759c3ba33347ed712531bee05bf1cc_49_49

[mr-c]

Anything we can do on our side to help with bringing back the branchwater feature for the Debian package?

Restoring the branchwater feature in the Debian package of sourmash would require packaging the rocksdb crate for Debian: https://wiki.debian.org/Teams/RustPackaging#Packages

I've started this https://salsa.debian.org/rust-team/debcargo-conf/-/commit/019a36fb202e3005e8fe075b34c21bfeb0a41875