[scripts-audit] vcpkg_from_* #18272

strega-nil · 2021-06-04T20:06:06Z

This PR goes through the vcpkg_from_* functions and brings them in line with the script audit (while also making them far easier to read and much DRY-er, imo)

See issue #17691

This PR should likely be reviewed a commit at a time; or at least the first commit alone, the next three commits together, and then the latter two separately.

Still to do:

vcpkg_from_sourceforge
vcpkg_from_git

strega-nil · 2021-06-08T15:47:03Z

/azp run

azure-pipelines · 2021-06-08T15:47:08Z

Commenter does not have sufficient privileges for PR 18272 in repo microsoft/vcpkg

strega-nil-ms · 2021-06-08T15:53:55Z

/azp run

azure-pipelines · 2021-06-08T15:54:10Z

Azure Pipelines successfully started running 1 pipeline(s).

scripts/cmake/vcpkg_download_distfile.cmake

ports/qt5-base/portfile.cmake

scripts/cmake/vcpkg_download_distfile.cmake

scripts/cmake/vcpkg_from_git.cmake

ras0219-msft · 2021-06-09T22:30:50Z

scripts/cmake/vcpkg_from_github.cmake

+        URLS "${github_host}/${org_name}/${repo_name}/archive/${ref_to_use}.tar.gz"
+        FILENAME "${downloaded_file_name}"
+        ${headers}
+        ${sha512}
+        ${redownload}


Can we use the fully resolved VCPKG_HEAD_VERSION to avoid redownloading every time?

I don't know, given that we aren't checking the SHA512 that makes me uncomfortable. thoughts?

We also don't check the SHA512 of our tools (cmake, etc) every time we use them, so I don't think this is fundamentally different as long as we're using the full SHA1 commit id.

scripts/cmake/vcpkg_from_bitbucket.cmake

scripts/cmake/vcpkg_from_gitlab.cmake

scripts/cmake/vcpkg_from_sourceforge.cmake

strega-nil-ms · 2021-06-11T22:12:29Z

/azp run

azure-pipelines · 2021-06-11T22:12:37Z

Azure Pipelines successfully started running 1 pipeline(s).

strega-nil-ms · 2021-06-14T18:37:03Z

/azp run

azure-pipelines · 2021-06-14T18:37:11Z

Azure Pipelines successfully started running 1 pipeline(s).

strega-nil-ms · 2021-06-14T19:40:26Z

/azp run

azure-pipelines · 2021-06-14T19:40:34Z

Azure Pipelines successfully started running 1 pipeline(s).

this option is useful when doing HEAD installs

Cleans up vcpkg_from_bitbucket quite a lot. This should provide a basis for other vcpkg_from_* functions

This also adds support for --head, and replaces the old "X_OUT_REF" with `VCPKG_HEAD_VERSION`

this additionally deprecates the `SOURCEFORGE_MIRRORS` extension point, and adds a replacement in the form of `VCPKG_SOURCEFORGE_EXTRA_MIRRORS`

strega-nil-ms · 2021-06-24T20:28:25Z

/azp run

azure-pipelines · 2021-06-24T20:28:34Z

Azure Pipelines successfully started running 1 pipeline(s).

docs/regenerate.ps1

scripts/cmake/vcpkg_download_distfile.cmake

scripts/cmake/vcpkg_from_bitbucket.cmake

BillyONeal · 2021-06-28T21:41:16Z

scripts/cmake/vcpkg_from_git.cmake

+        if(VCPKG_USE_HEAD_VERSION)
+            set(VCPKG_HEAD_VERSION "${rev_parse_head}" PARENT_SCOPE)
+        elseif(NOT rev_parse_head STREQUAL arg_REF)
+            message(FATAL_ERROR "REF (${arg_REF}) does not match FETCH_HEAD (${rev_parse_head})


I think the expected/actual output being damaged here is a regression. Moreover, I'm not convinced that the 'this means' statement is an accurate summation of the issue?

This means that we asked git to fetch a ref, and the FETCH_HEAD is at a different SHA than we asked for, which should be a 'can't happen' thing, not a 'please use a full sha' thing.

This is not necessarily accurate:

> git fetch https://github.com/microsoft/vcpkg master > git rev-parse FETCH_HEAD <ref-of-master-right-now>

master != <rev-of-master-right-now>, which is what we want to avoid here.

Additionally, this is an existing behavior:

if(NOT REV_PARSE_HEAD STREQUAL _vdud_REF AND NOT DEFINED _vdud_X_OUT_REF) message(STATUS "[Expected : ( ${_vdud_REF} )]") message(STATUS "[ Actual : ( ${REV_PARSE_HEAD} )]") message(FATAL_ERROR "REF (${_vdud_REF}) does not match FETCH_HEAD (${REV_PARSE_HEAD})")

so I'm just changing the message.

master != , which is what we want to avoid here.

That can't be what this is actually looking for, because if it were, all named refs in the tree would be broken right now.
(Since their SHA is never going to be their name)

Additionally, this is an existing behavior:

Right, I'm saying I think the expected/actual part of the message was a good thing to have, and I don't think the part you added about REF not being a SHA is necessarily the cause of the problem. (Since REF not being a SHA is a very common / normal condition)

hmm, okay, I'll switch it back then.

For vcpkg_from_git(), we explicitly want to require REF to be a SHA1 to ensure we are not getting dynamic behavior outside of --head.

This check is a roundabout way to implement that and could probably use wording improvement.

BillyONeal

Approving despite outstanding comments because:

(1) first comment is about artifact caching, and I think Robert understands those things better than I anyway. Robert, please double check that you're happy with the implications there.

(2) the damaged 'no longer printing expected SHAs' output I think is from a 'can't happen' case

scripts/cmake/vcpkg_from_git.cmake

ras0219-msft · 2021-07-04T08:58:14Z

scripts/cmake/vcpkg_from_git.cmake

+        if(VCPKG_USE_HEAD_VERSION)
+            set(VCPKG_HEAD_VERSION "${rev_parse_head}" PARENT_SCOPE)
+        elseif(NOT rev_parse_head STREQUAL arg_REF)
+            message(FATAL_ERROR "REF (${arg_REF}) does not match FETCH_HEAD (${rev_parse_head})


For vcpkg_from_git(), we explicitly want to require REF to be a SHA1 to ensure we are not getting dynamic behavior outside of --head.

This check is a roundabout way to implement that and could probably use wording improvement.

scripts/cmake/vcpkg_from_gitlab.cmake

strega-nil-ms · 2021-07-06T17:02:41Z

Closed for rollup PR #18838

[scripts-audit] vcpkg_from_*

@strega-nil

* [rollup:2021-07-06 1/8] PR #18272 (@strega-nil) [scripts-audit] vcpkg_from_* * [rollup:2021-07-06 2/8] PR #18319 (@strega-nil) [scripts-audit] add guidelines for cmake * [rollup 2021-07-06 3/8] PR #18410 (@mheyman) [vcpkg-cmake-config] documentation fix * [rollup:2021-07-06 4/8] PR #18488 (@strega-nil) [scripts-audit] vcpkg_execute_* * [rollup:2021-07-06 5/8] PR #18517 (@strega-nil) [scripts-audit] vcpkg_extract_source_archive * [rollup:2021-07-06 6/8] PR #18674 (@NancyLi1013) [vcpkg doc] Update examples * [rollup:2021-07-06 7/8] PR #18695 (@JackBoosY) [vcpkg] Update the minimum version of vcpkg * [rollup:2021-07-06 8/8] PR #18758 (@ras0219-msft) [vcpkg_from_git] Fix error if downloads folder does not exist * build docs! * fix bond:*-windows * fix nmap Co-authored-by: nicole mazzuca <[email protected]> Co-authored-by: Michael Heyman <[email protected]> Co-authored-by: NancyLi1013 <[email protected]> Co-authored-by: JackBoosY <[email protected]> Co-authored-by: Robert Schumacher <[email protected]>

strega-nil mentioned this pull request Jun 4, 2021

[scripts-audit] Scripts tree auditing #17691

Closed

51 tasks

strega-nil-ms requested a review from ras0219-msft June 4, 2021 20:07

strega-nil-ms added the info:world-rebuild label Jun 4, 2021

strega-nil-ms force-pushed the vcpkg_from_ branch 6 times, most recently from de7e8be to 332c4fc Compare June 6, 2021 14:27

JonLiu1993 assigned PhoebeHui Jun 7, 2021

PhoebeHui added the category:scripts-audit Part of the scripts audit effort. label Jun 7, 2021

strega-nil-ms force-pushed the vcpkg_from_ branch 2 times, most recently from 00085e4 to 7033a2f Compare June 7, 2021 19:56

ras0219-msft reviewed Jun 9, 2021

View reviewed changes

PhoebeHui added the info:internal This PR or Issue was filed by the vcpkg team. label Jun 17, 2021

strega-nil added 5 commits June 23, 2021 08:45

[vcpkg_download_distfile] add ALWAYS_DOWNLOAD option

d6567fe

this option is useful when doing HEAD installs

[scripts-audit] vcpkg_from_bitbucket

1335a75

Cleans up vcpkg_from_bitbucket quite a lot. This should provide a basis for other vcpkg_from_* functions

[scripts-audit] vcpkg_from_github

eff0b1d

[scripts-audit] vcpkg_from_gitlab

7588465

[scripts-audit] vcpkg_from_git

9b0787a

This also adds support for --head, and replaces the old "X_OUT_REF" with `VCPKG_HEAD_VERSION`

strega-nil added 3 commits June 23, 2021 08:59

[scripts-audit] vcpkg_from_sourceforge

cd5cb22

this additionally deprecates the `SOURCEFORGE_MIRRORS` extension point, and adds a replacement in the form of `VCPKG_SOURCEFORGE_EXTRA_MIRRORS`

Robert CRs

4375c8d

docs

0dfdb30

strega-nil-ms force-pushed the vcpkg_from_ branch from 56492b8 to 0dfdb30 Compare June 23, 2021 15:59

add version file for qt5-mqtt

4d9b941

strega-nil-ms force-pushed the vcpkg_from_ branch from 3f3bc56 to 4d9b941 Compare June 28, 2021 17:04

BillyONeal reviewed Jun 28, 2021

View reviewed changes

docs/regenerate.ps1 Outdated Show resolved Hide resolved

BillyONeal reviewed Jun 28, 2021

View reviewed changes

scripts/cmake/vcpkg_download_distfile.cmake Show resolved Hide resolved

BillyONeal reviewed Jun 28, 2021

View reviewed changes

scripts/cmake/vcpkg_from_bitbucket.cmake Show resolved Hide resolved

BillyONeal reviewed Jun 28, 2021

View reviewed changes

BillyONeal approved these changes Jun 28, 2021

View reviewed changes

strega-nil added 3 commits June 28, 2021 14:59

billy CR

9cdac34

new guidelines around set

ddb7e8d

billy CR

d5b0ae0

strega-nil requested a review from ras0219-msft July 2, 2021 22:04

ras0219-msft approved these changes Jul 4, 2021

View reviewed changes

Robert CRs

ca5a065

strega-nil-ms mentioned this pull request Jul 6, 2021

[rollup:2021-07-06] Rollup PR #18838

Merged

strega-nil-ms closed this Jul 6, 2021

strega-nil-ms pushed a commit to strega-nil/vcpkg that referenced this pull request Jul 9, 2021

[rollup:2021-07-06 1/8] PR microsoft#18272 (@strega-nil)

dba4871

[scripts-audit] vcpkg_from_*

strega-nil deleted the vcpkg_from_ branch July 16, 2021 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[scripts-audit] vcpkg_from_* #18272

[scripts-audit] vcpkg_from_* #18272

strega-nil commented Jun 4, 2021 •

edited

Loading

strega-nil commented Jun 8, 2021

azure-pipelines bot commented Jun 8, 2021

strega-nil-ms commented Jun 8, 2021

azure-pipelines bot commented Jun 8, 2021

ras0219-msft Jun 9, 2021

strega-nil-ms Jun 9, 2021

ras0219-msft Jul 4, 2021

strega-nil-ms commented Jun 11, 2021

azure-pipelines bot commented Jun 11, 2021

strega-nil-ms commented Jun 14, 2021

azure-pipelines bot commented Jun 14, 2021

strega-nil-ms commented Jun 14, 2021

azure-pipelines bot commented Jun 14, 2021

strega-nil-ms commented Jun 24, 2021

azure-pipelines bot commented Jun 24, 2021

BillyONeal Jun 28, 2021

strega-nil-ms Jun 28, 2021

BillyONeal Jun 28, 2021

strega-nil-ms Jun 28, 2021

ras0219-msft Jul 4, 2021

BillyONeal left a comment

ras0219-msft Jul 4, 2021

strega-nil-ms commented Jul 6, 2021

[scripts-audit] vcpkg_from_* #18272

[scripts-audit] vcpkg_from_* #18272

Conversation

strega-nil commented Jun 4, 2021 • edited Loading

strega-nil commented Jun 8, 2021

azure-pipelines bot commented Jun 8, 2021

strega-nil-ms commented Jun 8, 2021

azure-pipelines bot commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strega-nil-ms commented Jun 11, 2021

azure-pipelines bot commented Jun 11, 2021

strega-nil-ms commented Jun 14, 2021

azure-pipelines bot commented Jun 14, 2021

strega-nil-ms commented Jun 14, 2021

azure-pipelines bot commented Jun 14, 2021

strega-nil-ms commented Jun 24, 2021

azure-pipelines bot commented Jun 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strega-nil-ms commented Jul 6, 2021

strega-nil commented Jun 4, 2021 •

edited

Loading