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_download_distfile] Pad the SHA to re-enable testing the hash #18285

Merged

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Jun 5, 2021

No description provided.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added 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. labels Jun 7, 2021
@ras0219-msft ras0219-msft force-pushed the dev/roschuma/fixup-sha512-length branch from 78e0891 to 5a55cb8 Compare June 9, 2021 21:24
@autoantwort
Copy link
Contributor

Yeah but now it doesn't work anymore if I use 1 as SHA, or something else

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Jun 9, 2021

That's true, but I think for the purposes of local development it is not a burden to require use of the literal 0 (esp. compared with the burden of synthesizing exactly 128 0's). Most importantly, the error messages should make the path forward clear.

@autoantwort
Copy link
Contributor

That's true, but I think for the purposes of local development it is not a burden to require use of the literal 0 (esp. compared with the burden of synthesizing exactly 128 0's).

The code to fill up the hash with zeros up to a string length of 128 was not that complicated.

Most importantly, the error messages should make the path forward clear.

No, it only shows that I have entered an invalid sha, e.g. for the input 1. Imho not very user friendly. Imho it should just do it like in your first version, it is the easiest one for the users.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil-ms
Copy link
Contributor

Thanks again robert!

@strega-nil-ms strega-nil-ms merged commit abcaa4b into microsoft:master Jun 11, 2021
@autoantwort
Copy link
Contributor

When you stick with the only 0 way you should update the documentation here, because there you say that users should use 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

6 participants