-
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
[scripts-audit] CMake buildsystem!!! #16377
[scripts-audit] CMake buildsystem!!! #16377
Conversation
ports/fmt/portfile.cmake
Outdated
@@ -6,16 +8,17 @@ vcpkg_from_github( | |||
HEAD_REF master | |||
PATCHES fix-warning4189.patch | |||
) | |||
vcpkg_configure_cmake( | |||
vcpkg_cmake_buildsystem_configure( |
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.
If you need another name why not vcpkg_cmake_configure
.
Also scripts need to be versioned so having vcpkg_configure_cmake
use cmake_language(CALL <command> [<arg>...])
and have command be something like vcpkg_configure_cmake_<version>
. The reason behind this is that probably multiple versions of the scripts are required.
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.
scripts somehow does not work well with the single version behavior vcpkg currently has if you want to support multiple versions of ports which might depend on different script versions.
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 does not make any sense to me.
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.
port1#V1 depends on scripts#V4; port2#V15 depends on scripts#V2;
port1#V1 does not build with scripts#V2 and port2#V15 does not build with scripts#V4 (because somebody broke backcompat) for some reason (port2#V50 does but that does not matter here.)
scripts are a bit different to normal port dependencies, in my opinion. You can have multiple version of scripts at the same time if you version the folders|files|functions without bigger problems.
ff57480
to
0f2e7a5
Compare
a6e90be
to
47ac11f
Compare
* pull the cmake doc comment parsing out into its own function * support cmake helper ports * add real support for deprecation, as opposed to ad-hoc
Deprecate `vcpkg_*_cmake` in favor of `vcpkg_cmake_*` from the `vcpkg-cmake` port, as well as `vcpkg_fixup_cmake_targets` in favor of `vcpkg_cmake_config_fixup` from the `vcpkg-cmake-config` port.
47ac11f
to
7299ab1
Compare
Closing in favor of a single PR that contains all of these PRs. (to avoid destroying the CI system) |
Deprecate `vcpkg_*_cmake` in favor of `vcpkg_cmake_*` from the `vcpkg-cmake` port, as well as `vcpkg_fixup_cmake_targets` in favor of `vcpkg_cmake_config_fixup` from the `vcpkg-cmake-config` port.
* [scripts-audit rollup] PR #16419 * pull the cmake doc comment parsing out into its own function * support cmake helper ports * add real support for deprecation, as opposed to ad-hoc * [scripts-audit rollup] PR #16192 * add a z_ in front of internal functions * move internal functions out set feature_vars again in parent scope * [scripts-audit rollup] PR #16309 Audit vcpkg_copy_pdbs * [scripts-audit rollup] PR #16304 * Fix usage, documentation * [scripts-audit rollup] PR #16393 * [scripts-audit rollup] PR #16377 Deprecate `vcpkg_*_cmake` in favor of `vcpkg_cmake_*` from the `vcpkg-cmake` port, as well as `vcpkg_fixup_cmake_targets` in favor of `vcpkg_cmake_config_fixup` from the `vcpkg-cmake-config` port.
# Ninja and MSBuild have many differences when targetting UWP, so use MSBuild to maximize existing compatibility | ||
set(ninja_can_be_used OFF) |
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.
That breaks the behavor, see #19818
See PR #16055, issue #16188
Depends on PR #16419
Part of the "merge 2021-02-26 EOD" group.
cc @ras0219, @Neumann-A for review