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.

Sign up for GitHub

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

Jump to bottom

[many ports]switch to vcpkg-cmake / vcpkg-cmake-config part 2 #29882

Merged
merged 31 commits into from
Apr 21, 2023
Merged

[many ports]switch to vcpkg-cmake / vcpkg-cmake-config part 2 #29882

merged 31 commits into from
Apr 21, 2023

Conversation

JackBoosY
Copy link
Contributor

Prev: #29880

@JackBoosY JackBoosY mentioned this pull request Feb 27, 2023
Merged
@MonicaLiu0311 MonicaLiu0311 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:code-cleanup This PR cleans up code, without fixing any existing bugs nor adding any features. and removed category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Feb 27, 2023
@JackBoosY JackBoosY mentioned this pull request Feb 28, 2023
Open
@JackBoosY JackBoosY mentioned this pull request Feb 28, 2023
Merged
@JackBoosY
Copy link
Contributor Author

JackBoosY commented Feb 28, 2023 •
edited
Loading

depends on #28084

@JackBoosY
Copy link
Contributor Author

@JackBoosY
Can you separate the failed ports to different PRs?

There is no rush, so we can wait for the dependent PRs to be merged.

@talregev
Copy link
Contributor

talregev commented Mar 1, 2023

@JackBoosY
Can you separate the failed ports to different PRs?

There is no rush, so we can wait for the dependent PRs to be merged.

Can you put a links for dependent PRs that you are waiting for this PR? Put them in the top comment will help.
Thank you for your hard work!

@JackBoosY
Copy link
Contributor Author

@JackBoosY
Can you separate the failed ports to different PRs?

There is no rush, so we can wait for the dependent PRs to be merged.

Can you put a links for dependent PRs that you are waiting for this PR? Put them in the top comment will help. Thank you for your hard work!

Once #28084 was merged, I will merge this PR to master.

@JackBoosY JackBoosY mentioned this pull request Mar 7, 2023
Closed
@MonicaLiu0311
Copy link
Contributor

@JackBoosY
Convert this PR to draft since there is no progress. Please ping us if this PR is ready for review again.

@MonicaLiu0311 MonicaLiu0311 marked this pull request as draft March 21, 2023 10:52
@BillyONeal BillyONeal marked this pull request as ready for review April 19, 2023 04:03
@BillyONeal
Copy link
Member

#26981 covered a lot of the same ground and as a result some of the changes that are left only add quotes. Do you still think we should have those changes or should we drop the merely quote changes?

BillyONeal
BillyONeal reviewed Apr 19, 2023
View reviewed changes
ports/functions-framework-cpp/portfile.cmake
file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/debug/include)
vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake PACKAGE_NAME share)
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/include")
vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake)
Copy link
Member

@BillyONeal BillyONeal Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note that #26981 added PACKAGE_NAME share which seems wrong so I resolved the conflict in favor of the changes here.

BillyONeal
BillyONeal previously approved these changes Apr 19, 2023
View reviewed changes
@JackBoosY
Copy link
Contributor Author

@BillyONeal We still need to switch vcpkg_fixup_cmake_targets to vcpkg_config_cmake_fixup.
This PR is preparing for #29830 and #30070 so we can continue PR microsoft/vcpkg-tool#675

@BillyONeal
Copy link
Member

@BillyONeal We still need to switch vcpkg_fixup_cmake_targets to vcpkg_config_cmake_fixup. This PR is preparing for #29830 and #30070 so we can continue PR microsoft/vcpkg-tool#675

I don't see any left:

vcpkg_fixup_cmake_targets search results

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Apr 19, 2023 •
edited
Loading

@BillyONeal We still need to switch vcpkg_fixup_cmake_targets to vcpkg_config_cmake_fixup. This PR is preparing for #29830 and #30070 so we can continue PR microsoft/vcpkg-tool#675

I don't see any left:

vcpkg_fixup_cmake_targets search results

ports/ignition-modularscripts/vcpkg-port-config.cmake , so we needs to merge #29916 first.

@talregev
Copy link
Contributor

@JackBoosY Do you still insist not to separate this port?

@JackBoosY
Copy link
Contributor Author

@JackBoosY Do you still insist not to separate this port?

Why do we need to separate them?

@talregev
Copy link
Contributor

talregev commented Apr 19, 2023 •
edited
Loading

@JackBoosY Do you still insist not to separate this port?

Why do we need to separate them?

For the ports that not connected to the errors and not need to wait for other PRs, can be fixed and be on the master.

@JackBoosY
Copy link
Contributor Author

@JackBoosY Do you still insist not to separate this port?

Why do we need to separate them?

For the ports that not connected to the errors and not need to wait for other PRs, can be fixed and be on the master.

But they are done for fix.

@MonicaLiu0311 MonicaLiu0311 changed the title [many ports]switch to vcpkg-cmake / vckg-cmake-config part 2 [many ports]switch to vcpkg-cmake / vcpkg-cmake-config part 2 Apr 21, 2023
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Apr 21, 2023
@BillyONeal BillyONeal merged commit 7cfd63d into microsoft:master Apr 21, 2023
@BillyONeal
Copy link
Member

Thank you!

@JackBoosY JackBoosY deleted the dev/jack/cleanup_vcpkg_fixup_cmake_targets_2 branch April 22, 2023 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Osyotr Osyotr Osyotr left review comments

@MonicaLiu0311 MonicaLiu0311 MonicaLiu0311 approved these changes

@BillyONeal BillyONeal BillyONeal left review comments

Assignees

@MonicaLiu0311 MonicaLiu0311

Labels
category:code-cleanup This PR cleans up code, without fixing any existing bugs nor adding any features. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@JackBoosY @talregev @MonicaLiu0311 @BillyONeal @Osyotr