-
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
add virtual-libjpeg recipe #14814
add virtual-libjpeg recipe #14814
Conversation
This comment has been minimized.
This comment has been minimized.
So I've tested to update These 2 commands lead to the same libtiff package id, it's a serious blocker (it was my main concern explained in conan-io/conan#12374):
|
0d5e43f
to
a2f591f
Compare
This comment has been minimized.
This comment has been minimized.
self.requires(f"{self.options.implementation}/{self._impl_version}", transitive_headers=True, transitive_libs=True) | ||
|
||
def package_id(self): | ||
self.info.clear() |
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.
I think you need to clear everything but options.
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.
You mean as a workaround for #14814 (comment)?
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.
Yes, but after reading it again. I'm not sure anymore.
This virtual recipe must "push" its options to its users.
I see parallels with INTERFACE from cmake.
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.
The main issue is what I've mentioned in conan-io/conan#12374:
I see one main issue with such virtual recipe and current conan behavior: if a recipeA depends on this virtual recipe, the underlying recipe implementation is not anymore a direct dependency of recipeA, so I suspect package id issues.
How to design this virtual recipe so that any recipe having it as a direct dependency would behave as if either libjpeg, libjpeg-turbo or mozjpeg would be considered a direct dependency?
Hi @SpaceIm - thanks for taking the time to look into this. We have added this to our team backlog for further discussion and will provide feedback here once we determine a way forward. May I ask: other than the inconsistencies at the recipe level (which are, indeed, an issue) - have we observed concrete issues where a resolved dependency graph ends up pulling both libjpeg and jpeg-turbo, potentially causing issues due to the same symbols being provided by multiple libraries? This would be my priority - ensure that consumers are not exposed to obscure linker errors. If that is indeed addressed, I'd err on the side of providing the fastest jpeg implementation they can get without causing experiencing issues.
On the surface of it, I think this would work well if the jpeg library implementations are compatible and replacements of each other - this would potentially ensure that for any resolved dependency graph, there is only one jpeg implementation, avoiding those potential symbol collections I mention above. However, for the particular case of the jpeg libraries, I'm not sure this cover all cases:
So at the moment I'm not entirely sure, because there are scenarios in which they're not interchangeable. In Ubuntu, I can see that when libjpeg8 is requested, libjpeg turbo is installed, but libjpeg9 is its own, thus limiting this to libjpeg v8 and jpeg turbo. This proposal would go further than that. As mentioned in my previous reply, we will study this case and decide a way forward with the team! Very interesting case at hand. |
Conan v1 pipelineFailure in build 3 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. Conan v2 pipeline (informative, not required for merge)Failure in build 3 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
It can't happen, this kind of dependency graph raises, because
Yes it might be an issue, it needs more investigation. AFAIK it works fine in vcpkg.
Not a problem, if someone wants to use turbojpeg library, it can still explicitly add libjpeg-turbo (or mozjpeg) to requirements, it's not mutually exclusive with this virtual recipe, which is only about libjpeg. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
@jcar87 is there already some decision how to proceed? I'm interested in it to be able to use new ITK. It uses zlib-ng instead of zlib, but depends on libraries that use zlib. I want to be able to replace zlib with zlib-ng in compatibility mode. |
closes #2894
The recipe aims to take responsibility of libjpeg implementation selection in a dependency graph, so that we can avoid to pollute recipes with inconsistent
with_jpeg
/jpeg
options in conan-center recipes, recipes which also try to force these options in upstream recipes to avoid libjpeg implementation discrepancies, quite ugly.For example see current mess in opencv 3.x recipe:
conan-center-index/recipes/opencv/3.x/conanfile.py
Line 24 in ac2b0df
conan-center-index/recipes/opencv/3.x/conanfile.py
Lines 80 to 82 in ac2b0df
Therefore the next step will be to replace libjpeg/libjpeg-turbo requirement in all conan-center recipes by this one.
Once all recipes updated, it will guarantee consistency of libjpeg implementation in any dependency graph based on CCI recipes.
I've decided to select
libjpeg-turbo
as default implementation, it's the one used in most package managers.It's worth noting that
find_package(JPEG)
orpkg_check_modules(... libjpeg)
will work out of the box in downstream recipes, regardless of selected implementation, sincelibjpeg-turbo
generates proper files withCMakeDeps
/cmake_find_package
&PkgConfigDeps
/pkg_config
(since #13745). We just need to also fixmozjpeg-turbo
recipe (EDIT: fixed in #14825).TODO:
libjpeg
&libjpeg-turbo
? conan#12374)/cc @memsharded @jcar87 @czoido @danimtb @uilianries @prince-chrismc @SSE4