-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
qpdf/11.1.1 add (lib only) package #13113
qpdf/11.1.1 add (lib only) package #13113
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Mh it seems there is an requirement from within qpdf to require a more modern cmake version that installed on the ci-runners, since they report:
What is the way here to go? I an upgrade on ci-systems necessary? That would wonder me since other packages are using cmake version 3.16 in there test_package, like in boost-ext-ut/all/test_package/CMakeLists.txt. So how such a package was able to merge? Would it be a solution to add cmake from conan in a more modern version as a build_requirement? |
@cguentherTUChemnitz You should add cmake as tool_requires, in build_requirements() method: https://docs.conan.io/en/latest/migrating_to_2.0/recipes.html#requirements |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution. Please, refine the template according to your target project, remove unused methods and comments.
Thanks for your pleasant examples and documentations as well as your fast and detailed feedback ;) I will do it shortly |
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit e9d6be8qpdf/11.1.0
|
This comment has been minimized.
This comment has been minimized.
@uilianries I tried to update everything as your review suggest.
Do you think this is now in a mergeable state? |
f41e227
to
c776ad1
Compare
@uilianries fine that way?
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@uilianries Are we finished here? Can we trigger additional people to provide the necessary reviews? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
aecf7bf
This comment has been minimized.
This comment has been minimized.
… finally fix windows plattform of not enabling openssl sources, even when conan does provide openssl.
381e705
Sry guys for all the patch-rounds here, but i do not have a windows platform at hand, so i have to do quite some ping-pong with the ci. |
This comment has been minimized.
This comment has been minimized.
@uilianries What is here to do, when crucial dependency like the default used openssl is missing here on ci as prebuilt package? |
This comment has been minimized.
This comment has been minimized.
@uilianries @prince-chrismc I think we are finally here in a mergeable state with openssl conan support for all major platforms ;) |
recipes/qpdf/all/conanfile.py
Outdated
if self.options.with_ssl == "openssl": | ||
# TODO: update to 3.x branch of openssl, when zlib is able to handle it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the dependencies the other way around? 😖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A yes, your are right. I think it it not necessary to add the comment here after all, since updating dependencies is like an on-going todo... I would just now remove the comment to solve the discussion here.
@@ -119,12 +124,24 @@ def generate(self): | |||
|
|||
def _patch_sources(self): | |||
apply_conandata_patches(self) | |||
# we generally expect to have one crypto in-place, but need to patch the found mechanics | |||
# since we avoid currently the correct pkg_config | |||
replace_in_file(self, os.path.join(self.source_folder, "libqpdf", "CMakeLists.txt"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can go in the parch I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh, it is possible, but i would like to have the handling for the FOUND_CRYPTO, USE_CRYPTO_OPENSSL, USE_CRYPTO_GNUTLS, USE_CRYPTO_NATIVE next to each other, since they are already contextwise near to each other. Since the use_crypto_xxx flags are easier handled in the conanfile.py, since they are dependending on the conan options, i would like to keep the FROUND_CRYPTO also there. Is that fine for you, @prince-chrismc ?
All green in build 59 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Separating that patch could be a good improvement, but it's good enough now.
Specify library name and version: qpdf/11.1.0
I am not a maintainer of the qpdf project, but i would like to have it in the conan package ecosystem. The project speaks best on its-self:
https://github.com/qpdf/qpdf
The project provides cli tools as well as c++ and c libraries.
One explanation for the decision to go for libjpeg-turbo as hardcoded requirement:
libjpeg produces runtime errors in the package-test with:
This seems that i would have to downgrade libjpeg to something that is not present on the conan repository. Nevertheless i was happy to find out that libjpeg-turbo was just working fine.