-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
[libxml2,libxslt] Revise dependency handling #24935
Conversation
if you are already touching |
I will check the suggestion. I have more changes:
|
Align with CMake build system defaults.
Removing Adding I really don't know what is more robust in general, but IMO |
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 is a new experimental fast check for PR issues. Please let us know if this bot is helpful!
PRs must add only one version and must not modify any published versions
When making any changes to a library, the version or port-version in vcpkg.json
or CONTROL
must be modified.
error: checked-in files for libxslt have changed but the version was not updated
version: 1.1.35#1
old SHA: 57c65cc1e575ef799b76c703e96bb48344118ba4
new SHA: 0871a88f0f9e95c81944d2aafc26328d7e6cec80
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***
If it(libxslt) doesn't need |
This is what I say for our patches. But can I make this decision for upstream?
True, but OTOH we need to patch custom find modules quite often anyways (multi-config, debug postfix, platform support, transitive usage requirements, downstream support). With an official CMake Find module and an official exported config, I really don't need another custom find module... |
Yes since they don't need the |
ports/libxslt/fix-gcrypt-deps.patch
Outdated
if(LIBXSLT_WITH_CRYPTO) | ||
- find_package(Gcrypt REQUIRED) | ||
+ find_package(PkgConfig REQUIRED) | ||
+ pkg_check_modules(Gcrypt REQUIRED IMPORTED_TARGET libgcrypt) |
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.
+ pkg_check_modules(Gcrypt REQUIRED IMPORTED_TARGET libgcrypt) | |
+ pkg_check_modules(xslt_gcrypt REQUIRED IMPORTED_TARGET libgcrypt) |
better name this to something unique
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.
also better move the find_package(PkgConfig REQUIRED)
into the FindGcrypt.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.
I dislike patching and installing the custom find module.
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 dislike patching and installing the custom find module.
I understand that but patching FindGcrypt.cmake
makes it upstream-able if the code is guarded by if(PkgConfig_FOUND)
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.
From trying to build openscap
after installing libxslt[crypto]
:
No matter where I implement pkg_check_modules
: The maintainer functions for CMake don't set PKG_CONFIG_PATH
, so it doesn't don't find vcpkg's libgcrypt
. (And for most other consumers, it is only needed at configuration time: These port use libxslt, not libexslt.)openscap
and
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.
openscap uses libgcrypt and libexslt, and it comes with another load of custom find modules. Oh no.
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.
From trying to build
openscap
after installinglibxslt[crypto]
:
No matter where I implementpkg_check_modules
: The maintainer functions for CMake don't setPKG_CONFIG_PATH
, so it doesn't don't find vcpkg'slibgcrypt
.
The maintainer functions for CMake don't need to set PKG_CONFIG_PATH
when we can rely on PKG_CONFIG_USE_CMAKE_PREFIX_PATH
being initialized to true. But this requires CMAKE_MINIMUM_REQUIRED_VERSION
to be at least 3.1.
openscap
comes with CMAKE_MINIMUM_REQUIRED_VERSION
set to 2.6. Since this is a particular issue of this port, I decided to set PKG_CONFIG_USE_CMAKE_PREFIX_PATH
in that portfile.
I've noticed that libxslt installs shell script into |
Probabyly the scripts are not broken because they are explicitly handled by the portfile, and I often verify such items. But the toolchain is not adding |
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 have modified or added at least one portfile where deprecated functions are used.
If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake
-> vcpkg_cmake_install
(from port vcpkg-cmake
)
vcpkg_build_cmake
-> vcpkg_cmake_build
(from port vcpkg-cmake
)
vcpkg_configure_cmake
-> vcpkg_cmake_configure
(Please remove the option PREFER_NINJA
) (from port vcpkg-cmake
)
vcpkg_fixup_cmake_targets
-> vcpkg_cmake_config_fixup
(from port vcpkg-cmake-config
)
In the ports that use the new function, you have to add the corresponding dependencies:
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}
The following files are affected:
ports/openscap/portfile.cmake
You have modified or added at least one vcpkg.json where you should check the license
field.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/openscap/vcpkg.json
Valid values for the license field can be found in the documentation
Found and fixed prefix issues. |
Update: there is |
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 have modified or added at least one portfile where deprecated functions are used.
If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake
-> vcpkg_cmake_install
(from port vcpkg-cmake
)
vcpkg_build_cmake
-> vcpkg_cmake_build
(from port vcpkg-cmake
)
vcpkg_configure_cmake
-> vcpkg_cmake_configure
(Please remove the option PREFER_NINJA
) (from port vcpkg-cmake
)
vcpkg_fixup_cmake_targets
-> vcpkg_cmake_config_fixup
(from port vcpkg-cmake-config
)
In the ports that use the new function, you have to add the corresponding dependencies:
{
"name": "vcpkg-cmake",
"host": true
},
{
"name": "vcpkg-cmake-config",
"host": true
}
The following files are affected:
ports/openscap/portfile.cmake
You have modified or added at least one vcpkg.json where you should check the license
field.
If you feel able to do so, please consider adding a "license" field to the following files:
ports/openscap/vcpkg.json
Valid values for the license field can be found in the documentation
"Requires team review" again? Feedback, please. |
LGTM |
When building
In share/libxml2/libxml2-export.cmake: set(_targetsDefined)
set(_targetsNotDefined)
set(_expectedTargets)
foreach(_expectedTarget LibXml2::LibXml2 LibXml2::xmlcatalog LibXml2::xmllint)
list(APPEND _expectedTargets ${_expectedTarget})
if(NOT TARGET ${_expectedTarget})
list(APPEND _targetsNotDefined ${_expectedTarget})
endif()
if(TARGET ${_expectedTarget})
list(APPEND _targetsDefined ${_expectedTarget})
endif()
endforeach()
if("${_targetsDefined}" STREQUAL "${_expectedTargets}")
unset(_targetsDefined)
unset(_targetsNotDefined)
unset(_expectedTargets)
set(CMAKE_IMPORT_FILE_VERSION)
cmake_policy(POP)
return()
endif()
if(NOT "${_targetsDefined}" STREQUAL "")
message(FATAL_ERROR "Some (but not all) targets in this export set were already defined.\nTargets Defined: ${_targetsDefined}\nTargets not yet defined: ${_targetsNotDefined}\n")
endif()
unset(_targetsDefined)
unset(_targetsNotDefined)
unset(_expectedTargets) |
Did you really test with the last version of port libxml2? I remember that this was an issue in the past, when the find module was loaded before the config. But now the wrapper always loads the config. Just because of this issue. |
.. okay, reproduced. |
tmx bug: include(FindLibXml2)
find_package(LibXml2 REQUIRED) |
What does your PR fix?
Feature
crypto
:Crypto support in libxslt requires gcrypt, so port
libgcrypt
is an unconditional dependency of featurecrypto
.But port
libgcrypt
doesn't support mingw ATM, socrypto
cannot be a default feature formingw
.libgcrypt also needs libgpg-error. This is not handled by the embedded Find module, so this PR uses pkg-config instead.
Crypto support is opt-in the CMake build system. That's why this PR removes the
crypto
feature from the default features.Misc:
Feature
python
fails to build formingw
(on Linux), too. So it is marked as unsupported now.Removes unused direct dependency
liblzma
.Disables installation of documentation.
Revises the
libxml2
wrapper to resolve lookup and config mode quirks. (Related: find_package should not return system installed version if same package is installed from vcpkg #24995, [Part1|xwindow PR] Split up to dbus #22642 (comment).)Fixes
openscap
build issues due to vendoredFindThreads.cmake
.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