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

[pcre2] fetch from github #21193

Merged
merged 2 commits into from
Nov 5, 2021
Merged

[pcre2] fetch from github #21193

merged 2 commits into from
Nov 5, 2021

Conversation

m-kuhn
Copy link
Contributor

@m-kuhn m-kuhn commented Nov 4, 2021

Describe the pull request

  • What does your PR fix?

Fixes current build errors of pcre2 because https://ftp.pcre.org is not reachable. This only changes the download source of the code.
According to https://www.pcre.org/ the home is now github, so there we go get it now.

This does not change the version even though a new one is available but that fails to build at the moment. Step by step.

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/pcre2/portfile.cmake

@JonLiu1993 JonLiu1993 assigned PhoebeHui and JonLiu1993 and unassigned PhoebeHui Nov 5, 2021
@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Nov 5, 2021
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Nov 5, 2021
@dan-shaw dan-shaw merged commit afc6992 into microsoft:master Nov 5, 2021
@dg0yt dg0yt mentioned this pull request Nov 5, 2021
@m-kuhn m-kuhn deleted the pcre2_github branch November 5, 2021 23:24
@Canvis-Me
Copy link

Hello, I got pcre2 hash check error.

Building package pcre2[core]:x64-windows...
-- Downloading https://github.com/PhilipHazel/pcre2/archive/pcre2-10.37.tar.gz -> PhilipHazel-pcre2-pcre2-10.37.t
ar.gz...
[DEBUG] Feature flag 'binarycaching' unset
[DEBUG] Feature flag 'manifests' = off
[DEBUG] Feature flag 'compilertracking' unset
[DEBUG] Feature flag 'registries' unset
[DEBUG] Feature flag 'versions' unset
[DEBUG] Downloading https://github.com/PhilipHazel/pcre2/archive/pcre2-10.37.tar.gz
[DEBUG] D:\a_work\1\s\src\vcpkg\base\downloads.cpp(656)
Error: Failed to download from mirror set:
File does not have the expected hash:
url : [ https://github.com/PhilipHazel/pcre2/archive/pcre2-10.37.tar.gz ]
File path : [ D:\vcpkg\downloads\PhilipHazel-pcre2-pcre2-10.37.tar.gz.17232.part ]
Expected hash : [ f91760a8e0747f52211612fb0e134d685e224d16bd884eb574718d077a586b1fd7b6435d4e3b75c879b12e02b252
467ecc28cdc4bc2903c783dacab089f99c99 ]
Actual hash : [ f15357d8f5400739f26a32fe03a0759607fd7a282a1a8a2bb169b971a8a00330eff3ba465842c4668ad968e3fd1f
ff4dc7f0631d0af8416947070ff7cd056e6f ]

[DEBUG] Exiting after %s (%d us)

@c72578
Copy link
Contributor

c72578 commented Nov 6, 2021

Remark: This should not have passed CI. The difference of the SHA512 between the zip file and the tar.gz must be detected.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 6, 2021

This should not have passed CI.

I guess this went unnoticed due to "asset caching". If the cache key is just the file name, changes in content don't invalidate the cache. This affects local usage (downloads directory) as well CI.

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Nov 6, 2021

This should not have passed CI.

I guess this went unnoticed due to "asset caching".

Before it was a .zip, now a .tar.gz, that shouldn't be affected by caching.
I'm not currently at a keyboard and web interface only won't help with x-add-version.
If someone else could provide a fix that'd be great.

@JonLiu1993
Copy link
Member

I will submit pr to fix this bug

@cenit
Copy link
Contributor

cenit commented Nov 8, 2021

Remark: This should not have passed CI. The difference of the SHA512 between the zip file and the tar.gz must be detected.

couldn't agree more

@BillyONeal
Copy link
Member

Remark: This should not have passed CI. The difference of the SHA512 between the zip file and the tar.gz must be detected.

couldn't agree more

The problem is that part of the reason for asset caching is to prevent a failure of an external server from preventing all PR progress, and to prevent very very slow external servers from bringing the big build machines to a halt waiting to download those pieces.

I would love to have the best of both worlds somehow but I don't see obvious solutions.

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Nov 10, 2021

Remark: This should not have passed CI. The difference of the SHA512 between the zip file and the tar.gz must be detected.

couldn't agree more

The problem is that part of the reason for asset caching is to prevent a failure of an external server from preventing all PR progress, and to prevent very very slow external servers from bringing the big build machines to a halt waiting to download those pieces.

I would love to have the best of both worlds somehow but I don't see obvious solutions.

Maybe I'm missing something, but wouldn't it help to make the filename or download url the cache key? Currently it's the file basename or port version?

@BillyONeal
Copy link
Member

Maybe I'm missing something, but wouldn't it help to make the filename or download url the cache key? Currently it's the file basename or port version?

Currently it is just the SHA. It was intentionally not tied to a given URI because there are often multiple sources for a given thing. There is probably a middle ground that can be struck here but it's not super obvious...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants