-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[enkits] New port #34277
[enkits] New port #34277
Conversation
Note: I will be converting your PR to draft status. When you're ready, please revert to "ready for review". |
Please provide the usage file. |
It should be good to go. |
The name case and location issues of the config file:
Edit: solved. |
The usage test passed on
|
+ ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}) | ||
+ install(FILES ${ENKITS_HEADERS} DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/enkiTS) | ||
+ install( | ||
+ EXPORT unofficial-enkiTS-config |
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 creates an invalid file name. You musts not use upper-case with -config.cmake
. CMake only looks for <lower-case-package>config.cmake
. And we do want lower-case unofficial-<port>
package names.
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.
Out of curiosity, why did you accept this PR then? I fail to see the difference with mine in this case.
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.
... unexpected. I didn't even notice that it was a second PR. I only noticed that "unofficial" was "gone" again, and I found the config was added upstream.
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 case-sensitivity problem is real: #34487, dougbinks/enkiTS#103.
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.
Ok, thanks for the information and the fix. I used uppercase because that's what was done upstream (unreleased yet) and I hoped to be able to remove the patch on the next release, but I didn't know this could be an issue for CMake.
+ install(FILES ${ENKITS_HEADERS} DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/enkiTS) | ||
+ install( | ||
+ EXPORT unofficial-enkiTS-config | ||
+ NAMESPACE unofficial::enkiTS:: |
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 also want lower-case unofficial::<port>::
here.
@@ -0,0 +1,27 @@ | |||
vcpkg_check_linkage(ONLY_STATIC_LIBRARY) |
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.
Doesn't upstream support shared linkage on non-win32?
|
||
vcpkg_cmake_install() | ||
vcpkg_copy_pdbs() | ||
vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/unofficial-enkiTS PACKAGE_NAME unofficial-enkiTS) |
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.
... lower-case ...
find_package(unofficial-enkits CONFIG REQUIRED) | ||
target_link_libraries(main PRIVATE unofficial::enkiTS::enkiTS) |
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.
unofficial::enkits::enkiTS
+ install( | ||
+ EXPORT unofficial-enkiTS-config | ||
+ NAMESPACE unofficial::enkiTS:: | ||
+ DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/unofficial-enkiTS) |
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.
+ DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/unofficial-enkiTS) | |
+ DESTINATION share/unofficial-enkits) |
|
||
vcpkg_cmake_install() | ||
vcpkg_copy_pdbs() | ||
vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/unofficial-enkiTS PACKAGE_NAME unofficial-enkiTS) |
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.
vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/unofficial-enkiTS PACKAGE_NAME unofficial-enkiTS) | |
vcpkg_cmake_config_fixup(PACKAGE_NAME unofficial-enkits) |
I think this was done by #34386 ? |
Yes, it's the same with minor differences due to inconsistent reviews. I'm closing it. |
Duplicate of #34386. |
Add new port enkits : https://github.com/dougbinks/enkiTS
find_package
calls are REQUIRED, are satisfied byvcpkg.json
's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxxvcpkg.json
matches what upstream says.vcpkg.json
matches what upstream says../vcpkg x-add-version --all
and committing the result.