-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[geos] Relocatable geos-config; dynamic builds on Unix #17616
Conversation
ports/geos/portfile.cmake
Outdated
string(REGEX REPLACE "(-lgeos(_c)?)d?( |\n)" "\\1d\\3" GEOS_CONFIG "${GEOS_CONFIG}") | ||
file(WRITE "${CURRENT_PACKAGES_DIR}/debug/bin/geos-config" "${GEOS_CONFIG}") | ||
file(MAKE_DIRECTORY ${CURRENT_PACKAGES_DIR}/share/${PORT}) | ||
file(RENAME ${CURRENT_PACKAGES_DIR}/debug/bin/geos-config ${CURRENT_PACKAGES_DIR}/share/${PORT}/geos-config-debug) |
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.
Can you give an example of how this would be consumed? My understanding is that xyz-config
scripts like this are usually referred to like "${XYZ_ROOT}/xyz-config"
and so having xyz-config-debug
won't be directly usable. Instead, we should provide debug/share/geos/geos-config
and set GEOS_ROOT
to debug/share/geos
when in debug mode.
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.
gdal configure
says:
--with-geos=ARG Include GEOS support (ARG=yes, no or geos-config
path)
and similar for curl, kea, sfcgal.
So this is to be consumed in the gdal portfile by:
list(APPEND OPTIONS_RELEASE --with-geos=${CURRENT_INSTALLED_DIR}/share/geos/geos-config)
list(APPEND OPTIONS_DEBUG --with-geos=${CURRENT_INSTALLED_DIR}/share/geos/geos-config-debug)
I tested this locally, but this just one out of a pile of pending changes.
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.
provide
debug/share/geos/geos-config
and set set GEOS_ROOT to debug/share/geos
If I adopt this: Outside vcpkg, the <package>_ROOT
variable generally (e.g. in CMake find modules) means the installation prefix, i.e. ${CURRENT_INSTALLED_DIR}
.
As a consumer in a port file, I would still have to assign the roots to per-build-type parameters because the build types are handled deeper down in the cmake call stack.
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.
provide
debug/share/geos/geos-config
I tried that when preparing the PR, and I verified today: It leads into a build error:
-- Performing post-build validation
/debug/share should not exist. Please reorganize any important files, then use
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/share")
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.
Which file is in ${CURRENT_PACKAGES_DIR}/debug/share?
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.
Which file is in ${CURRENT_PACKAGES_DIR}/debug/share?
None with the current PR. @ras0219-msft proposed to have debug/share/geos/geos-config
, and I reported what would be the result.
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 alternative to fixing geos-config, curl-config etc. is forcing the consumers to use pkg-config where possible, by removing these files. Consumers also need to know whether to ask the scripts for static or standard flags, and this is solved for PKG_CONFIG
.
E.g. port gdal will need a number of patches anyway, e.g. to use pkg-config for tiff.
Ping @ras0219-msft for review this PR again. |
More side notes:
|
Will add pc file handling soon. |
Fixup pc files. Add -lgeos to Libs.private to satisfy common usage requirement (already present in geos-config). Install executable geos-config into tools. Rewrite geos-config relocatability as patch.
New changes:
|
Cool, LGTM! Thanks @dg0yt :) |
What does your PR fix?
This PR
geos-config
relocatable, and adds ageos-config-debug
for the debug build,as a step towards solving the gdal port issues ([GDAL] On Linux completly broken #9068, [gdal] linux build does not get dependencies from vcpkg #8943)
Which triplets are supported/not supported? Have you updated the CI baseline?
unchanged, no
Does your PR follow the maintainer guide?
yes
If you have added/updated a port: Have you run
./vcpkg x-add-version --all
and committed the result?yes
I still need to test the latest state with my gdal branch, but it is ready for vcpkg CI. The question in #17607 are related, too.