Skip to content
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

Closed

Conversation

strega-nil
Copy link
Contributor

@strega-nil strega-nil commented Feb 23, 2021

See PR #16055, issue #16188

Depends on PR #16419

Part of the "merge 2021-02-26 EOD" group.

cc @ras0219, @Neumann-A for review

@strega-nil strega-nil marked this pull request as draft February 23, 2021 17:54
ports/fmt/CONTROL Outdated Show resolved Hide resolved
ports/fmt/portfile.cmake Outdated Show resolved Hide resolved
@@ -6,16 +8,17 @@ vcpkg_from_github(
HEAD_REF master
PATCHES fix-warning4189.patch
)
vcpkg_configure_cmake(
vcpkg_cmake_buildsystem_configure(
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

ports/fmt/portfile.cmake Outdated Show resolved Hide resolved
@strega-nil strega-nil added category:scripts-audit Part of the scripts audit effort. info:internal This PR or Issue was filed by the vcpkg team. depends:different-pr This PR or Issue depends on a PR which has been filed labels Feb 23, 2021
@strega-nil strega-nil force-pushed the scripts-audit/cmake-buildsystem branch from ff57480 to 0f2e7a5 Compare February 25, 2021 22:46
@strega-nil strega-nil marked this pull request as ready for review February 25, 2021 22:52
@strega-nil strega-nil force-pushed the scripts-audit/cmake-buildsystem branch 2 times, most recently from a6e90be to 47ac11f Compare February 25, 2021 23:00
* 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.
@strega-nil strega-nil force-pushed the scripts-audit/cmake-buildsystem branch from 47ac11f to 7299ab1 Compare February 26, 2021 20:34
@strega-nil
Copy link
Contributor Author

Closing in favor of a single PR that contains all of these PRs. (to avoid destroying the CI system)

@strega-nil strega-nil closed this Feb 26, 2021
strega-nil added a commit to strega-nil/vcpkg that referenced this pull request Feb 27, 2021
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.
strega-nil added a commit that referenced this pull request Feb 28, 2021
* [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.
@strega-nil strega-nil deleted the scripts-audit/cmake-buildsystem branch March 1, 2021 18:46
Comment on lines +128 to +129
# Ninja and MSBuild have many differences when targetting UWP, so use MSBuild to maximize existing compatibility
set(ninja_can_be_used OFF)
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:scripts-audit Part of the scripts audit effort. depends:different-pr This PR or Issue depends on a PR which has been filed info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants