-
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
yder: add yder/1.4.18 recipe #13614
yder: add yder/1.4.18 recipe #13614
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
All green in build 4 (
|
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.
Would be nice to get some of the CMake-related patches upstreamed.
|
||
-target_link_libraries(yder ${LIBS} ${ORCANIA_LIBRARIES} ${SYSTEMD_LIBRARIES}) | ||
+if (BUILD_SHARED) | ||
+ target_link_libraries(yder PRIVATE $<TARGET_NAME:Orcania::Orcania> ${SYSTEMD_LIBRARIES}) |
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.
Does this mean that a shared build of yder
can't link to a static orcania
package?
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.
Not with the cmake script unmodified.
It is patched in _patch_sources()
.
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.
Gotcha.
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.
When/if conan-io/conan#12367 lands, the replace_in_file
can be replaced with that.
+ NAMESPACE "Yder::" | ||
+ FILE "YderTargets.cmake") | ||
+ | ||
+configure_package_config_file(cmake-modules/YderConfig.cmake.in YderConfig.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.
We should not be adding install or config names 😕 where does this come from?
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 pr I created.
It's stalling because the maintainer wants to understand the goal of the xxConfig.cmake
, xxConfig-targets.cmake
and xxConfigVersion.cmake
files (see orcania repo).
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.
Since this is not accepted upstream, I do think we should be adding them here 🙈
We should only expose official targets (its fine if they are added in a later release) but we should not be doing it here first
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.
Agreed. I'll mark this pr as draft until upstream makes a move.
The guy in charge is welcoming to the idea, so I have good feelings about this.
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 pr has been accepted. So please give this pr a final review.
Specify library name and version: yder/1.4.18
Depends on #13589