Skip to content
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

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

Merged
merged 5 commits into from
Mar 5, 2024
Merged

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

merged 5 commits into from
Mar 5, 2024

Conversation

adamreichold
Copy link
Contributor

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.

Copy link
Owner

@ancwrd1 ancwrd1 left a comment

Choose a reason for hiding this comment

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

I am actually wondering if it should be other way around, adding a 'vendored' feature and making a native build the default one.
It's a breaking change though (so requires a version bump), but consistent with other crates.

@adamreichold
Copy link
Contributor Author

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
Copy link
Owner

ancwrd1 commented Mar 4, 2024

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
Copy link
Owner

ancwrd1 commented Mar 4, 2024

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
Copy link
Contributor Author

adamreichold commented Mar 4, 2024

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.

…df 10.x and 11.x in the default configuration.
@adamreichold
Copy link
Contributor Author

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
Copy link
Owner

ancwrd1 commented Mar 4, 2024

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
Copy link
Owner

ancwrd1 commented Mar 5, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

ancwrd1 commented Mar 5, 2024

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

@ancwrd1
Copy link
Owner

ancwrd1 commented Mar 5, 2024

Great, thanks for your contribution!

@ancwrd1 ancwrd1 merged commit 16dd529 into ancwrd1:master Mar 5, 2024
4 checks passed
@adamreichold
Copy link
Contributor Author

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
Copy link
Owner

ancwrd1 commented Mar 6, 2024

Done, version 0.2.0 is published.

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.

2 participants