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

[scripts-audit] vcpkg.cmake #16061

Merged
merged 11 commits into from
Feb 19, 2021
Merged

[scripts-audit] vcpkg.cmake #16061

merged 11 commits into from
Feb 19, 2021

Conversation

strega-nil
Copy link
Contributor

@strega-nil strega-nil commented Feb 5, 2021 •
edited
Loading

See PR #16055

This also (attempts to) allow vcpkg.cmake to run with cmake 3.1; we had a VERSION_GREATER_EQUAL check, but that if keyword was added in cmake 3.7.

@strega-nil strega-nil force-pushed the x_z_variables branch 2 times, most recently from 20f4464 to 0a6efac Compare February 5, 2021 22:50
@Neumann-A
Copy link
Contributor

I would really like to see some cleanup first instead of more things added to vcpkg.cmake.

@PhoebeHui PhoebeHui self-assigned this Feb 7, 2021
@PhoebeHui PhoebeHui added info:internal This PR or Issue was filed by the vcpkg team. category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed labels Feb 7, 2021
ras0219-msft
ras0219-msft reviewed Feb 8, 2021
View reviewed changes
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

The pieces I've reviewed LGTM so far.

Still to review:

  • New macro/functions
  • find_package() override

@strega-nil strega-nil mentioned this pull request Feb 8, 2021
Merged
@strega-nil strega-nil added the depends:different-pr This PR or Issue depends on a PR which has been filed label Feb 8, 2021
@strega-nil strega-nil marked this pull request as draft February 8, 2021 23:39
@strega-nil
Copy link
Contributor Author

strega-nil commented Feb 8, 2021 •
edited
Loading

Depends on #16130 merged

@strega-nil
[vcpkg.cmake] modify to get in line with the audit
13138fd 
See PR microsoft#16055

This also (attempts to) allow vcpkg.cmake to run with cmake 3.0; we had
a VERSION_GREATER_EQUAL check, but that if keyword was added in cmake
3.7.
@strega-nil
s/target_name/package_name in find_package
3b0dd5c
@strega-nil
depend on z_vcpkg_function_arguments
99b60ae 
instead of a custom macro
also, as a drive-by, switch to foreach(X IN LISTS ...)
@strega-nil
split the utility macros/functions out
e83e7bb 
additionally, move the options and settings to the top of the file
@strega-nil strega-nil removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Feb 10, 2021
@strega-nil strega-nil marked this pull request as ready for review February 10, 2021 00:02
@strega-nil
move more options and settings to the top
3d2b04e
@strega-nil
fix name of file
9f56110
@strega-nil
fix ras code comments
bf328da 
* remove z_vcpkg_utilities due to export
* add cmake_policy(PUSH|POP)
* add VCPKG_INSTALLED_DIR input variable
* add .cmakestamp to vcpkg_installed
@strega-nil
fix osx ifs to use quotes
0a0ded6 
also, remove spaces between if and (, to keep style consistent
(mostly for ease of grepping...)
@strega-nil
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil strega-nil mentioned this pull request Feb 12, 2021
Open
@strega-nil strega-nil added the category:scripts-audit Part of the scripts audit effort. label Feb 12, 2021
@strega-nil strega-nil changed the title [vcpkg.cmake] modify to get in line with audit [scripts-audit] vcpkg.cmake Feb 12, 2021
@strega-nil
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

BillyONeal
BillyONeal approved these changes Feb 18, 2021
View reviewed changes
scripts/buildsystems/vcpkg.cmake Show resolved Hide resolved
scripts/buildsystems/vcpkg.cmake Show resolved Hide resolved
scripts/buildsystems/vcpkg.cmake Show resolved Hide resolved
scripts/buildsystems/vcpkg.cmake Show resolved Hide resolved
@strega-nil strega-nil merged commit 1bb5ea1 into microsoft:master Feb 19, 2021
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Feb 19, 2021
@Jimmy-Hu
Merge pull request #541 from microsoft/master
caf8754 
[scripts-audit] vcpkg.cmake (microsoft#16061)
@cenit
Copy link
Contributor

cenit commented Feb 19, 2021 •
edited
Loading

@strega-nil Did you try the vcpkg.cmake script after the re-work (audit?) on a downstream project?
It looks like it broke vcpkg totally. No packages can be found. I can reproduce it locally and it is being triggered right now in every ci pipeline of mine.
A message is also just appeared about that: #15956 (comment)

@cenit cenit mentioned this pull request Feb 19, 2021
Merged
@strega-nil strega-nil mentioned this pull request Feb 19, 2021
Merged
@strega-nil strega-nil deleted the x_z_variables branch March 1, 2021 18:47
@strega-nil strega-nil mentioned this pull request May 5, 2021
Closed
51 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes

@Neumann-A Neumann-A Neumann-A left review comments

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

Assignees

@PhoebeHui PhoebeHui

Labels
category:scripts-audit Part of the scripts audit effort. category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@strega-nil @Neumann-A @cenit @BillyONeal @ras0219-msft @PhoebeHui