Skip to content

Visual Studio solution: check for updates of vcpkg #2424

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

Closed
wants to merge 1 commit into from
Closed

Visual Studio solution: check for updates of vcpkg #2424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ jobs:
(New-Object Net.WebClient).DownloadFile($downloadUrl, "compat.zip")
Expand-Archive compat.zip -DestinationPath . -Force
Remove-Item compat.zip
new-item -path "$(Build.SourcesDirectory)/compat/vcbuild" -name NoPrebuildEvent.txt -type "file" -value "Inhibit git.sln PrebuildEvent"
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect.

What we need to figure out is not whether we run in a CI/PR build, but whether we can update vcpkg or not.

I am very much against introducing this change, as it is a clear "papering over" the actual problem: we are currently not figuring out whether vcpkg is initialized via cloning or, say, by copying the ready-built artifacts or not.

displayName: 'Download vcpkg artifacts'
- task: MSBuild@1
inputs:
Expand Down
32 changes: 25 additions & 7 deletions compat/vcbuild/vcpkg_install.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,16 @@ REM ================================================================
@FOR /F "delims=" %%D IN ("%~dp0") DO @SET cwd=%%~fD
cd %cwd%

dir vcpkg\vcpkg.exe >nul 2>nul && GOTO :install_libraries
REM the architecture is currently hard-coded, and that we will change that once we need to.
SET arch=x64-windows
Copy link
Member

Choose a reason for hiding this comment

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

Is this really the correct thing to do? I'd rather have this as independent of the current architecture as possible.

Copy link
Author

Choose a reason for hiding this comment

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

Is this really the correct thing to do? I'd rather have this as independent of the current architecture as possible.

It's the current way. That appears to be just the way it is (see lower down in the bat file).

Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned below, if we are already moving it, we should move it to the top, and add a good comment about the raison d'être for this variable.

Copy link
Member

Choose a reason for hiding this comment

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

Is this really the correct thing to do? I'd rather have this as independent of the current architecture as possible.


REM is vcpkg package installed and complied?
REM can be inhibited by creating compat\vcbuild\NoPrebuildEvent.txt

IF EXIST "vcpkg\installed\%arch%\include\openssl\ssl.h" GOTO :check_for_update

REM is vcpkg package manager already cloned
Copy link
Member

Choose a reason for hiding this comment

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

Please fix the indentation.

Copy link
Author

Choose a reason for hiding this comment

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

Local REM indentation is single space

Copy link
Member

Choose a reason for hiding this comment

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

Look at it on GitHub. There is a difference between the indentation of all your REMs vs the IF EXIST line. It's totally jagged.

IF EXIST vcpkg\vcpkg.exe GOTO GOTO :install_libraries

git.exe version 2>nul
IF ERRORLEVEL 1 (
Expand All @@ -53,9 +62,18 @@ REM ================================================================
IF ERRORLEVEL 1 ( EXIT /B 1 )

echo Successfully installed %cwd%vcpkg\vcpkg.exe
GOTO :install_libraries
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should not squeeze the check_for_updates "routine" between the part of the script where vcpkg.exe is built and install_libraries? That this extra goto is now required all of a sudden is a pretty clear indicator that this is the wrong location for check_for_updates. It would be much better to move it to the end of the script, as it is a pretty independent piece of new code.


:check_for_updates
echo Checking for vcpkg updates and upgrades to installed libraries
cd %cwd%vcpkg
git pull
.\vcpkg.exe update
.\vcpkg.exe upgrade --no-dry-run

EXIT /B 0

:install_libraries
SET arch=x64-windows

echo Installing third-party libraries...
FOR %%i IN (zlib expat libiconv openssl libssh2 curl) DO (
Expand All @@ -80,11 +98,11 @@ REM ================================================================
:sub__install_one
echo Installing package %1...

REM vcpkg may not be reliable on slow, intermittent or proxy
REM connections, see e.g.
REM https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/4a8f7be5-5e15-4213-a7bb-ddf424a954e6/winhttpsendrequest-ends-with-12002-errorhttptimeout-after-21-seconds-no-matter-what-timeout?forum=windowssdk
REM which explains the hidden 21 second timeout
REM (last post by Dave : Microsoft - Windows Networking team)
REM vcpkg may not be reliable on slow, intermittent or proxy
REM connections, see e.g.
REM https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/4a8f7be5-5e15-4213-a7bb-ddf424a954e6/winhttpsendrequest-ends-with-12002-errorhttptimeout-after-21-seconds-no-matter-what-timeout?forum=windowssdk
REM which explains the hidden 21 second timeout
REM (last post by Dave : Microsoft - Windows Networking team)

.\vcpkg.exe install %1:%arch%
IF ERRORLEVEL 1 ( EXIT /B 1 )
Expand Down
5 changes: 2 additions & 3 deletions contrib/buildsystems/Generators/Vcxproj.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,8 @@ sub createProject {
EOM
if ($target eq 'libgit') {
print F << "EOM";
<PreBuildEvent Condition="!Exists('$cdup\\compat\\vcbuild\\vcpkg\\installed\\\$(VCPKGArch)\\include\\openssl\\ssl.h')">
<Message>Initialize VCPKG</Message>
<Command>del "$cdup\\compat\\vcbuild\\vcpkg"</Command>
<PreBuildEvent Condition="!Exists('$cdup\\compat\\vcbuild\\NoPrebuildEvent.txt')">
Copy link
Member

Choose a reason for hiding this comment

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

Really, this is the wrong way to go about it.

<Message>Initialize or update VCPKG</Message>
<Command>call "$cdup\\compat\\vcbuild\\vcpkg_install.bat"</Command>
</PreBuildEvent>
EOM
Expand Down