-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
OpenColorIO: Fix dynamic linking for OpenColorIO >= 2.3.1 & fix package_info #23112
Conversation
…e tools build without linking error
🤖 Beep Boop! This pull request is making changes to 'recipes/opencolorio//'. 👋 @irieger you might be interested. 😉 |
This comment has been minimized.
This comment has been minimized.
1225c96
to
38d395c
Compare
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.
Hi! Thanks a lot! I have a small question regarding the CMakeLists change :)
|
||
target_link_libraries(OpenColorIO | ||
- PRIVATE | ||
+ PUBLIC # Change to public so that the tools properly link |
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.
Has this been submitted upstream? This looks like something that would better live as an official change upstream, where it can be properly maintained
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'm going to put up a pull request, but I'm currently working on getting into the CLA for the Linux Foundation which the upstream is part of. But I can also start the pull request later without the CLA standing and retrigger the check later. (And then reference the PR in the conandata)
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.
@irieger If you have a blocker with CLA because your job (for instance), don't worry, we can send the patch too. Thank you for checking 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.
No, the job is not a problem. It is more the other way round. Trying to get the CLA through my company (we have a company CLA there) as I don't like the Linux foundation requireing a physical address, which is why I refuse to sign it with my personal details. (An open source focused organisation like that having such a bad privacy statement is a red flag for me when asked for personal details.)
Me doing this is no problem for my company and although this contributions here are for my personal interest, I guess it is good to be active in open source projects from a company perspective anyway and I have some colleagues contributing to the Academy Software Foundation projects (ASFW is hosted - or whatever the right english word is here - by the Linux Foundation).
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.
Created a PR: AcademySoftwareFoundation/OpenColorIO#1958
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.
As this is anyway only a small part of the overall compatibility conan patching, I guess it doesn't make sense to add the upstream URL maybe?
Conan v1 pipeline ✔️All green in build 5 (
Conan v2 pipeline ✔️
All green in build 5 (
|
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.
Thanks for the contribution @irieger -
I'm not 100% changing the private to public is the right solution. By this I mean that it may solve the problem, but it may not be the right solution :)
Any chance we can get a reproducible example of the problems you are running into? including the actual build errors that you are getting?
So that we can verify this and ensure it is the right solution (it's also entirely possible that this is a Conan limitation, which I'm currently investigating).
#23421 is basically what I have as analysis and understanding. Logically to me the public is wrong too, but for some reason the Path of dependencies of Minizip isn't passed in non-public linking to the downstream executables. (The other alternative that works is manually adding minizip to every one of the executables that fails linking.) |
Trying to extract the relevant parts with #24395 |
Specify library name and version: opencolorio/>=2.3.1
I'm running into problems building my project with dynamic linking and one of the two culprits is OpenColorIO which currently prevents real dynamic linking and doesn't advertise its dependencies.