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

[vcpkg scripts] Remove deprecate functions vcpkg_*_cmake and vcpkg_fixup_cmake_targets #30070

Conversation

JackBoosY
Copy link
Contributor

Remove these deprecated functions completely since no port uses them.

@JackBoosY
Copy link
Contributor Author

Depends on #29882 and #29883.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 7, 2023

You cannot remove the function names. User ports may depend on them, and in manifest mode, even vcpkg ports will need them forever.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Mar 7, 2023

You cannot remove the function names. User ports may depend on them, and in manifest mode, even vcpkg ports will need them forever.

Good point.
However, there is no way to call vcpkg_cmake_* / vcpkg_cmake_config_fixup in those functions.
These functions are just a burden for vcpkg.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 7, 2023

Yes, it is technical debt which cannot be removed (except for a major release of vcpkg).
Fortunately, the new GH checks should ensure that these functions are not used in new ports.

@JackBoosY
Copy link
Contributor Author

At least we need a point in time to notify the user that they are about to be removed, like normal 3rd party libraries do.
Let's see what the maintainer team thinks.

@Neumann-A
Copy link
Contributor

You cannot remove the scripts due to versioning?

@JackBoosY
Copy link
Contributor Author

You cannot remove the scripts due to versioning?

Wouldn't versioning affect these scripts?

@dg0yt
Copy link
Contributor

dg0yt commented Mar 7, 2023

You cannot remove the scripts due to versioning?

These are the unversioned global scripts. Manifest mode allows to install old port versions which rely on these scripts.
There is more such legacy stuff, like some find_package particularities in vcpkg.cmake which cannot be remove despite better wrappers in current port versions.

@Neumann-A
Copy link
Contributor

I mean we could remove them if vcpkg-tool adds an implicit dep to a vcpkg-script port but that comes with its own set of advantages and disadvantages

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @dg0yt .

@LilyWangLL LilyWangLL added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Mar 8, 2023
@JackBoosY
Copy link
Contributor Author

Okay, so we need to modify our GitHub actions to report error when PR changes contains those deprecated functions right?

@dg0yt
Copy link
Contributor

dg0yt commented Mar 8, 2023

Maybe upgrade selected keywords from warning to error.

if [ -s .github-pr.changed-portfiles ]; then (grep -n -H -E '(vcpkg_apply_patches|vcpkg_build_msbuild|vcpkg_extract_source_archive_ex|vcpkg_install_cmake|vcpkg_build_cmake|vcpkg_configure_cmake|vcpkg_fixup_cmake_targets)' $(cat .github-pr.changed-portfiles) || true) > .github-pr.deprecated-cmake; else touch .github-pr.deprecated-cmake; fi

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Mar 13, 2023

Close this PR since it's not appropriate.

@JackBoosY JackBoosY closed this Mar 13, 2023
@JackBoosY JackBoosY deleted the dev/jack/deprecate-vcpkg-cmake-and-fixup branch March 13, 2023 02:46
@dg0yt
Copy link
Contributor

dg0yt commented Mar 13, 2023

You could also issue a warning at the top of each deprecated function.

@JackBoosY
Copy link
Contributor Author

You could also issue a warning at the top of each deprecated function.

Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants