search

ancwrd1 / qpdf-rs

Rust bindings for QPDF C++ library
17 stars 7 forks source link

RFC: Add optional support for building against the system library using pkg-config. #8

Closed adamreichold closed 8 months ago

adamreichold commented 8 months ago

qpdf is a pretty big build requiring vendored copies of zlib and libjpeg so it would be nice to link against the system library instead. I understand that there a no guarantees on compatibility, but for example my main target Ubuntu 22.04 currently ships qpdf 10.6.3 which appears to closely match what is vendored here. But even using qpdf 11.9.0 on OpenSUSE Tumbleweed, the only tests which break are for the apparently dropped r2/3/4 encryption options.

adamreichold commented 8 months ago

I am actually wondering if it should be other way around, adding a 'vendored' feature and making a native build the default one.

I am fine with it either way and can change accordingly if you'd like. Note that defaulting to the system library will yield compatibility issues for some though as e.g. qpdf 11.x seems to have removed some API that are available with 10.x.

I guess we should feature gate the older API as well then, so that fewer builds break. Personally, I would suggest disabling that feature by default and only offering the older encryption option variants if explicitly requested.

ancwrd1 commented 8 months ago

I am fine with it either way and can change accordingly if you'd like. Note that defaulting to the system library will yield compatibility issues for some though as e.g. qpdf 11.x seems to have removed some API that are available with 10.x.

I guess we should feature gate the older API as well then, so that fewer builds break. Personally, I would suggest disabling that feature by default and only offering the older encryption option variants if explicitly requested.

Sounds good to me.

ancwrd1 commented 8 months ago

There is actually a qpdf-11.6 branch with a pending work of making version 11 a vendored one. One little issue is that it now requires C++14 compiler which isn't available on older Linux systems (e.g. a very popular in the enterprise CentOS/RHEL 7). So I am a bit stuck with version 10, but perhaps this native build would at least partially solve it.

adamreichold commented 8 months ago

There is actually a qpdf-11.6 branch with a pending work of making version 11 a vendored one. One little issue is that it now requires C++14 compiler which isn't available on older Linux systems (e.g. a very popular in the enterprise CentOS/RHEL 7). So I am a bit stuck with version 10, but perhaps this native build would at least partially solve it.

Personally, I would prefer compatibility with both 10.x and 11.x because our servers would use 10.x (Ubuntu LTS), but our development machines use 11.x (Ubuntu 23.10). If apparently the R2/3/4 encryption options would be the only issue, then the cost of supporting both appears small at first sight.

adamreichold commented 8 months ago

Pushed a variant adding legacy and vendored features so that the default build should work with both 10.x and 11.x system libraries out of the box.

I did not touch the crate version or anything. Let me know if I should. I am also not sure what CI and/or documentation updates you would think appropriate for that change.

ancwrd1 commented 8 months ago

I am also not sure what CI and/or documentation updates you would think appropriate for that change.

May be a README update with a section about feature flags and add the 'vendored' feature in the CI script for now.

ancwrd1 commented 8 months ago

Something is not right with the CI builds, I am not sure why does it fail because the build script has --all-features in there.

adamreichold commented 8 months ago

Something is not right with the CI builds, I am not sure why does it fail because the build script has --all-features in there.

I think the problem is that the default build fails because libqpdf is not available in the containers (won't be easily become available in Windows and macOS).

I tried to add the necessary changes to make the CI use a vendored build where necessary and explicitly test a system build on Linux, but admittedly this is difficult to test for me due to the workflow approval requirement.

ancwrd1 commented 8 months ago

I think a sudo is probably needed for apt-get to work. AFAIK the workflows are using passwordless sudo.

ancwrd1 commented 8 months ago

Great, thanks for your contribution!

adamreichold commented 8 months ago

I would be grateful if you could release the system library support on crates.io so that we can integrate it downstream and give it gets more testing under realistic conditions. Thank you!

ancwrd1 commented 8 months ago

Done, version 0.2.0 is published.