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] Replace uses of msys pacman.exe with direct package downloads #13019

Merged
merged 11 commits into from
Sep 1, 2020

Conversation

ras0219
Copy link
Contributor

@ras0219 ras0219 commented Aug 20, 2020

This PR completely replaces the approach inside vcpkg_acquire_msys() with a more direct and streamlined package acquisition stream. Instead of relying on pacman.exe to get packages (which often changes out from underneath us, either by changing keys or otherwise), we instead encode direct references to the archives themselves. This results in a vastly more stable system, since every file is hash checked and goes through our existing download machinery, ensuring everything is properly pinned and robust. It also ensures each unique set of packages gets a unique directory, which ensures that separate builds will avoid destructive interference -- everyone will always get exactly what was requested and no more.

However, this also means we lose the ability for arbitrary ports to "simply" use packages we don't already know about (and perhaps didn't even exist at the time of vcpkg_acquire_msys()'s authoring). To handle this case, ports can instead directly fetch packages by adding them to the DIRECT_PACKAGES parameter; this will simply download the provided URLs and unpack them into the same msys directory. These URLs can be trivially found by perusing https://packages.msys2.org; see ffmpeg as an example.

@JackBoosY JackBoosY self-assigned this Aug 20, 2020
@JackBoosY JackBoosY added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:internal This PR or Issue was filed by the vcpkg team. labels Aug 20, 2020
@ras0219 ras0219 force-pushed the dev/roschuma/msys branch 3 times, most recently from 39396c4 to eb69df6 Compare August 21, 2020 07:50
Apply suggestions from code review

Co-authored-by: Alexander Neumann <30894796+Neumann-A@users.noreply.github.com>
@MVoz
Copy link
Contributor

MVoz commented Aug 22, 2020

this will greatly limit the addition of any utilities or updates, taking into account how fixes are made to the main branch
very, very dubious edits

IMHO

these edits can be made parallel to the main MSYS2?

@ras0219
Copy link
Contributor Author

ras0219 commented Aug 23, 2020

this will greatly limit the addition of any utilities or updates, taking into account how fixes are made to the main branch

@voskrese We definitely need to enable ports to use new utilities -- that is why I've added the DIRECT_PACKAGES parameter, which can be used by ports to privately access other packages without disrupting the stability of others. As an example, I've used it in the ffmpeg port:

https://github.com/microsoft/vcpkg/pull/13019/files#diff-426be71f8525e065de9ace89a5d4bf66R45-R60

@ras0219 ras0219 marked this pull request as ready for review August 24, 2020 19:00
@ras0219-msft ras0219-msft changed the title [vcpkg] WIP: replace uses of msys pacman.exe with direct package downloads [vcpkg] Replace uses of msys pacman.exe with direct package downloads Aug 26, 2020
@@ -12,5 +12,5 @@ Build-Depends: openblas
Description: Use external optimized BLAS

Feature: blas-select
Build-Depends: lapack-reference[core, noblas](!windows|(windows&!static))
Build-Depends: lapack-reference[core, noblas](!windows|!static)
Copy link
Contributor

Choose a reason for hiding this comment

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

I sincerely love these diff snippets in a PR, an unnecessary touch of logic and love out of the blue.

@emptyVoid
Copy link
Contributor

What about MSYS2 being a rolling release distro? Those direct links are not guaranteed to last.

function(tensorflow_try_remove_recurse_wait PATH_TO_REMOVE)
file(REMOVE_RECURSE ${PATH_TO_REMOVE})
if (EXISTS "${PATH_TO_REMOVE}")
execute_process(COMMAND ${CMAKE_COMMAND} -E sleep 5)
_execute_process(COMMAND ${CMAKE_COMMAND} -E sleep 5)
Copy link
Member

Choose a reason for hiding this comment

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

Why did we change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It enables this sleep to execute during download-only mode

@ras0219
Copy link
Contributor Author

ras0219 commented Aug 28, 2020

Based on https://repo.msys2.org/msys/x86_64/, it looks like packages are kept for at least 5 years. This is sufficient for our current needs, and in the future we will investigate adding mechanisms to mirror remote resources such as these.

ras0219 pushed a commit to ras0219/vcpkg that referenced this pull request Aug 30, 2020
…issues

As part of a full rebuild in microsoft#13019, several transitive rebuild issues were found.

1) selene was dynamically detecting the presence of OpenCV, which was not on its dependency list.
2) The dynamic OpenCV was using `find_package()` instead of `find_dependency()`
3) The dynamic OpenCV was always finding all dependencies instead of just those that were used during build.
4) CMake's FindHDF5 module requires C language support
5) ceres was passing `REQUIRED` into find_dependency (not needed -- find_dependency implies REQUIRED)
6) FindLAPACK.cmake was sometimes provided by the meta-port lapack, sometimes by the dependency lapack-reference/clapack
7) The lapack-reference vcpkg-cmake-wrapper was directly including the FindLAPACK, which doesn't respect variables such as QUIET and REQUIRED

Interestingly, the particular issue required a cascade of failures to occur to actually cause a build failure; almost any of these fixes would independently fix the build.
@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ras0219-msft ras0219-msft merged commit 46e25a1 into microsoft:master Sep 1, 2020
@linquize
Copy link

linquize commented Sep 1, 2020

This pull request uses cmake file() to download pkg-config

file(DOWNLOAD https://repo.msys2.org/mingw/i686/mingw-w64-i686-pkg-config-0.29.2-1-any.pkg.tar.xz)
file(DOWNLOAD https://repo.msys2.org/mingw/i686/mingw-w64-i686-libwinpthread-git-8.0.0.5906.c9a21571-1-any.pkg.tar.zst)

If using Windows 7, downloading from https://repo.msys2.org/ will fail with status: 35: SSL connect error
If using Windows 10 2004, downloading from https://repo.msys2.org/ will succeed.
I did not test Windows 8.1 or older Windows 10, not sure if they succeed or not.
The server https://repo.msys2.org/ is running TLS 1.3 HTTP/2

@BillyONeal
Copy link
Member

@linquize file(DOWNLOAD is how we download everything else in vcpkg; do you have suggestions?

@linquize
Copy link

linquize commented Sep 2, 2020

Seems cmake on Windows uses winhttp to download, see #13286

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly 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.

9 participants