Visual Studio solution: check for updates of vcpkg #2424

PhilipOakley · 2019-12-08T13:18:23Z

The Microsoft vcpkg package manager recieves continual updates. The
pre-built vs/master git.sln Visual Studio solution would clone and
compile the vcpkg dependencies if missing, but did not check for
updates. Fix this.

Update the contrib/buildsystems/Generators/Vcxproj.pm.

Remove the condition on 'Pre-Build' event for installation, and thus
always check for updates if the vcpkg manager is already present.

The `vcpkg_install.bat' file name is retained to avoid churn. The batch
file now also handles the vcpkg update check.

See microsoft/vcpkg#9148 for update documentation,
however the use of "./vcpkg" is a bashism, not suited to a cmd.bat file.
Use vcpkg, no need for the .exe part, removing the ./ should be
sufficient.

Signed-off-by: Philip Oakley [email protected]

I have moderately tested this, and it's working on my VS2017 community edition by pulling and checking for updates, however I don't think there's been any major updates while I've been testing.

This follows the mailing list discussion with @SyntevoAlex https://public-inbox.org/git/?q=%3C021de37a-5317-6c96-eae3-d0228a193d8b%40iee.email%3E

Suggestion for driving the tesing would be helpful (e.g. best way / how to spoof the initial clone to be from say March 2019, so that updates would be needed on second compile.)

PhilipOakley · 2019-12-08T16:48:49Z

Hmm two test fails, both for lacking vcpkg-root so not sure why that is. Maybe the pull request CI goes through a slightly different start process so that it doesn't show, but I'm, at the moment clueless why it's happened.

PhilipOakley · 2019-12-08T18:07:00Z

hmm still the same problem.

dscho

A good first draft, thank you!

dscho · 2019-12-11T09:39:18Z

compat/vcbuild/vcpkg_install.bat

@@ -34,6 +34,12 @@ REM ================================================================
 	@FOR /F "delims=" %%D IN ("%~dp0") DO @SET cwd=%%~fD
 	cd %cwd%

+	SET arch=x64-windows


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

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).

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.

dscho · 2019-12-11T09:39:18Z

compat/vcbuild/vcpkg_install.bat

@@ -34,6 +34,12 @@ REM ================================================================
 	@FOR /F "delims=" %%D IN ("%~dp0") DO @SET cwd=%%~fD
 	cd %cwd%

+	SET arch=x64-windows


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

dscho · 2019-12-11T09:39:31Z

compat/vcbuild/vcpkg_install.bat

@@ -34,6 +34,12 @@ REM ================================================================
 	@FOR /F "delims=" %%D IN ("%~dp0") DO @SET cwd=%%~fD
 	cd %cwd%

+	SET arch=x64-windows
+
+REM is vcpkg package installed and complied \\compat\\vcbuild\\vcpkg\\installed\\\$(VCPKGArch)\\include\\openssl\\ssl.h


Please fix the indentation.

indentation

The current file uses single space after the REM. Was there an issue with the existing file, or is there something I can't see in the Github gui?

dscho · 2019-12-11T09:41:20Z

compat/vcbuild/vcpkg_install.bat

+	SET arch=x64-windows
+
+REM is vcpkg package installed and complied \\compat\\vcbuild\\vcpkg\\installed\\\$(VCPKGArch)\\include\\openssl\\ssl.h
+	dir vcpkg\installed\%arch%\include\openssl\ssl.h >nul 2>nul && GOTO :check_for_updates


Oy. Should this not be IF EXIST "..." GOTO ...?

Relying on the exit status of dir when a file does, or does not exist, strikes me as odd in this context.

Oy, oy, oy. I just realized that you copy-edited code already in the script... Tsk, tsk. Still, there is no good reason to copy flawed code, let's use IF EXIST instead.

I'll try the IF EXIST approach. I never like success=0 with the wrong/poor && logic.

dscho · 2019-12-11T09:41:29Z

compat/vcbuild/vcpkg_install.bat

+REM is vcpkg package installed and complied \\compat\\vcbuild\\vcpkg\\installed\\\$(VCPKGArch)\\include\\openssl\\ssl.h
+	dir vcpkg\installed\%arch%\include\openssl\ssl.h >nul 2>nul && GOTO :check_for_updates
+
+REM is vcpkg package manager already cloned


Please fix the indentation.

Local REM indentation is single space

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.

dscho · 2019-12-11T09:43:24Z

compat/vcbuild/vcpkg_install.bat


 :install_libraries
-	SET arch=x64-windows
+REM	SET arch=x64-windows


Ah, that's where this comes from. Urgh.

So you are just moving the definition. But you're not, you're commenting out the old code, making it "dead code". Please don't.

And if you are moving the definition, please move it to the top, with a comment that the architecture is currently hard-coded, and that we will change that once we need to.

"Dead code" - I'll keep it (typically) until we have working code (the point of requesting the support/review).

the architecture is currently hard-coded, and that we will change that once we need to

I can add your comment.

dscho · 2019-12-11T09:53:41Z

Hmm two test fails, both for lacking vcpkg-root so not sure why that is. Maybe the pull request CI goes through a slightly different start process so that it doesn't show

From https://dev.azure.com/Git-for-Windows/git/_build/results?buildId=46958&view=logs&jobId=2e71db62-524c-5004-06b5-96b7fc531cee (lines 25--36):

PreBuildEvent:
  Description: Initialize or update VCPKG
  call "..\compat\vcbuild\vcpkg_install.bat"
  :VCEnd
  Checking for vcpkg updates and upgrades to installed libraries
  You are not currently on a branch.
  Please specify which branch you want to merge with.
  See git-pull(1) for details.
  
      git pull <remote> <branch>
  
##[error]EXEC(0,0): Error : Could not detect vcpkg-root.
EXEC : error : Could not detect vcpkg-root. [D:\a\1\s\libgit\libgit.vcxproj]

I would wager a bet that the problem is that we copy the vcpkg build artifacts from https://dev.azure.com/git/git/_build?definitionId=9 and unpack them here, and the .git directory is not copied.

The reason is that it would be super wasteful to build the vcpkg artifacts every single time that a CI/PR build runs in gitgitgadget, git or git-for-windows.

Therefore, you will want to verify that there is, in fact, a .git directory inside compat\vcbuild\vcpkg, and if there is not, skip your new step.

PhilipOakley · 2019-12-13T12:15:17Z

I would wager a bet that the problem is that we copy the vcpkg build artifacts from https://dev.azure.com/git/git/_build?definitionId=9 and unpack them here, and the .git directory is not copied.

Where is that all documented? I never found my way to anything that helped.

Isn't the 'right' solution for the CI to at least clone the repo shallow=1 to match the expected user experience? If the existing solution is retained how do other that fall into the same trap of an old vcpkg install find out /test what's happened.

dscho · 2019-12-13T13:35:56Z

I would wager a bet that the problem is that we copy the vcpkg build artifacts from https://dev.azure.com/git/git/_build?definitionId=9 and unpack them here, and the .git directory is not copied.

Where is that all documented? I never found my way to anything that helped.

It's in the CI definition: https://github.com/git-for-windows/git/blob/v2.24.0.windows.2/azure-pipelines.yml#L170-L177

Isn't the 'right' solution for the CI to at least clone the repo shallow=1 to match the expected user experience? If the existing solution is retained how do other that fall into the same trap of an old vcpkg install find out /test what's happened.

If you are saying that the "right" solution is to let vcpkg download and build all dependencies in all of those dozens of CI/PR builds that happen every day, then I heartily disagree.

PhilipOakley · 2019-12-13T18:22:32Z

If you are saying that the "right" solution is to let vcpkg download and build all dependencies in all of those dozens of CI/PR builds that happen every day, then I heartily disagree.

So what do you suggest, seeing as we are trying to implement a method (for the user) that does download, build, test for update, all the dependencies?

The current setup is a catch22 that avoids any of those user issues, and bypasses the user based tests.

Given your suggestion as to what's happening, it looks as if we currently (without up-to-date-ness checks) have a Prebuild-Event that tests for the existence of the compilation products, allowing the CI to simply dump a prebuilt set of binaries (etc) into the requisite directories, so that no pre-build event is executed, avoiding the batch file.

In the new (up-to-date-ness check) case the Prebuild-Event condition has to be dropped as the check is no longer a valid indicator of up-to-date-ness, leaving the CI to mimic the user scenario.

If you can suggest an alternate sentinel that could be added by the CI then maybe an alternate pre-build condition could be used (I see a remove line for a .zip indicating tweaks but...).

I have zero experience or knowledge of Azure pipelines (those referenced lines look like gobbledygook at the moment)

dscho · 2019-12-13T21:56:23Z

If you can suggest an alternate sentinel that could be added by the CI then maybe an alternate pre-build condition could be used (I see a remove line for a .zip indicating tweaks but...).

To work around the failure, you could make this conditional on either the vcpkg directory not existing or vcpkg\.git existing.

To be honest, I am not a huge fan of always trying to update the dependencies. Sounds too forced, and too costly, to me.

PhilipOakley · 2019-12-14T12:55:30Z

To be honest, I am not a huge fan of always trying to update the dependencies. Sounds too forced, and too costly, to me.

I'm also cautious that the cost should not be onerous (e.g. only one check per day or similar), but I do think that it can't be left forever without some form of dependency check.

we copy the vcpkg build artifacts from https://dev.azure.com/git/git/_build?definitionId=9 and unpack them here,

Are those build products also allowed to be very stale (half a year), or are the somehow auto built either by vcpkg or somewhere earlier in the git CI? (see earlier note about this being new territory for me, so a proper question)

To work around the failure, you could make this conditional on ...

I believe we do need the extra sentinel indicator that is special to the CI unpack, so we can use that in the VS PrebuildEvent. All the other options are between a rock and a hard place - we do need both the detections (of lack of vcpkg, and stale vcpkg) to also happen for the users machine, even if the stale vcpkg isn't an issue for the CI (see Q above).

I think I found that we could insert a [ps example!] command in the yml to create a vcpkg/NoPrebuildEvent.txt file, that replaces (stands in) the current if !exist() condition in the generators/vcxproj.pm, and consequently git.sln, so the CI never sees the vcpkg_install.bat, while the user does.

new-item -path C:\docs -name SS64.txt -type "file" -value "some text"

Obviously getting the path, name and text sorted ;-)

That then leaves the vcpkg_install.bat free to do any user based special checks for initial install, dependencies, and up to date(day), checks.

dscho · 2019-12-14T18:11:49Z

only one check per day or similar

It would seem that this should be possible (see SO post): something like Condition="!Exists('..\compat\vcbuild\vcpkg') || (Exists('..\compat\vcbuild\vcpkg\.git') && ($([System.DateTime]::Now().Ticks) > $([System.IO.File]::GetLastWriteTime('..\compat\vcbuild\vcpkg\.git\HEAD').Ticks))))"

This is untested, of course, as I wanted to leave a little bit of this work to you.

To work around the failure, you could make this conditional on ...

I believe we do need the extra sentinel indicator that is special to the CI unpack, so we can use that in the VS PrebuildEvent.

Well, there is one, and I already mentioned it: test for the presence of vcpkg without vcpkg\.git being present.

This would also have the further advantage of being the sentinel for the condition we actually care about: We couldn't care less whether this is a CI build or not. What we actually care about is whether we can update vcpkg or not.

The Microsoft vcpkg package manager recieves continual updates. The pre-built vs/master git.sln Visual Studio solution would clone and compile the vcpkg dependencies if missing, but did not check for updates. Fix this user-land issue. The Azure Continuous Integration (CI) system would pre-build the vcpkg files and expand them in place rather than clone and rebuild them. Unfortunately the CI logic and the update logic are in conflict as both detect/use the presence/absence of the same files via the git.sln Prebuild Event. The .git directory is not present in the Azure-CI, which conflicts with using a `git pull` update.. Add a sentinel file to the Azure-CI to indicate that the CI is in use, and use that as the git.sln conditional PrebuildEvent for checking for vcpkg updates in user-land. Update the contrib/buildsystems/Generators/Vcxproj.pm. Remove the condition on 'Pre-Build' event for installation, and thus always check for updates if the vcpkg manager is already present. The `vcpkg_install.bat' file name is retained to avoid churn. The batch file now also handles the vcpkg update check. See microsoft/vcpkg#9148 for update documentation, however the use of "./vcpkg" is a bashism, not suited to a cmd.bat file. Use `vcpkg`, no need for the .exe part, removing the `./` should be sufficient. Signed-off-by: Philip Oakley <[email protected]> --- corrected some bad logic and confusion that success is zero though logical true for the && cases, and the reverse sequence of existance checks. still contains residual artefacts for dev assistance.

PhilipOakley · 2019-12-14T18:45:12Z

I have Addressed what I can, and hacked other parts with best guesses.

Arguably the changes could be split to:

add the Azure sentinel file
change the prebuild event condition
update the vcpkg_install file to check for updates
(tbd) add a 'once per day' semaphore into /vcpkg/, maybe even in the .git.

PhilipOakley · 2019-12-14T20:40:57Z

Sorry, I missed your update that was just before I'd sent the hacked update (still some rough errors and leftover crud in the commit message, and it failed the tests..)

It would seem that this should be possible (see SO post): something like Condition="!Exists('..\compat\vcbuild\vcpkg') || (Exists('..\compat\vcbuild\vcpkg\.git') && ($([System.DateTime]::Now().Ticks) > $([System.IO.File]::GetLastWriteTime('..\compat\vcbuild\vcpkg\.git\HEAD').Ticks))))"

This is untested, of course, as I wanted to leave a little bit of this work to you.

Ok, I see where you are going there. I couldn't get my head around the checking logic before. The time check will need a tweak as it's almost always true (the last HEAD update was before Now() 😉 )

The one benefit of the sentinel was that a user could also set it, once they know about it.

One thing I don't see in VS, or the Azure builds is any reporting of the prebuild event condition. It doesn't appear to show in the logs or the VS properties - Do you see it?

PhilipOakley · 2019-12-14T22:04:52Z

it failed the tests.

Looks like it's now passed.
Seems odd that the VS build appears to fail in the first few minutes but then changes to passing later.

I'm away for a few days ...

dscho · 2019-12-15T17:19:13Z

azure-pipelines.yml

@@ -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"


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.

dscho · 2019-12-15T17:19:59Z

compat/vcbuild/vcpkg_install.bat

@@ -34,6 +34,12 @@ REM ================================================================
 	@FOR /F "delims=" %%D IN ("%~dp0") DO @SET cwd=%%~fD
 	cd %cwd%

+	SET arch=x64-windows


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.

dscho · 2019-12-15T17:20:36Z

compat/vcbuild/vcpkg_install.bat

+REM is vcpkg package installed and complied \\compat\\vcbuild\\vcpkg\\installed\\\$(VCPKGArch)\\include\\openssl\\ssl.h
+	dir vcpkg\installed\%arch%\include\openssl\ssl.h >nul 2>nul && GOTO :check_for_updates
+
+REM is vcpkg package manager already cloned


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.

dscho · 2019-12-15T17:24:18Z

compat/vcbuild/vcpkg_install.bat

@@ -53,9 +62,18 @@ REM ================================================================
 	IF ERRORLEVEL 1 ( EXIT /B 1 )

 	echo Successfully installed %cwd%vcpkg\vcpkg.exe
+	GOTO :install_libraries


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.

dscho · 2019-12-15T17:24:40Z

contrib/buildsystems/Generators/Vcxproj.pm

-    <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')">


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

dscho · 2020-10-06T09:44:41Z

@PhilipOakley I'll just close this, okay?

If you're still interested in something like it, it will most likely make more sense to integrate it somehow with the CMake support we just completed.

PhilipOakley requested review from dscho and jeffhostetler December 8, 2019 13:18

PhilipOakley force-pushed the vkpkg_check_gfw branch from 2a30265 to 56f8ce8 Compare December 8, 2019 17:54

dscho requested changes Dec 11, 2019

View reviewed changes

PhilipOakley force-pushed the vkpkg_check_gfw branch from 56f8ce8 to 1bf3708 Compare December 14, 2019 18:39

weekly-digest bot mentioned this pull request Dec 15, 2019

Weekly Digest (8 December, 2019 - 15 December, 2019) #2432

Closed

dscho requested changes Dec 15, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Dec 22, 2019

Weekly Digest (15 December, 2019 - 22 December, 2019) #2441

Closed

git-for-windows-ci changed the base branch from master to main June 17, 2020 18:10

dscho closed this Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visual Studio solution: check for updates of vcpkg #2424

Visual Studio solution: check for updates of vcpkg #2424

PhilipOakley commented Dec 8, 2019

PhilipOakley commented Dec 8, 2019

PhilipOakley commented Dec 8, 2019

dscho left a comment

dscho Dec 11, 2019

PhilipOakley Dec 13, 2019

dscho Dec 15, 2019

dscho Dec 11, 2019

dscho Dec 11, 2019

PhilipOakley Dec 13, 2019

dscho Dec 11, 2019

dscho Dec 11, 2019

PhilipOakley Dec 13, 2019

dscho Dec 11, 2019

PhilipOakley Dec 13, 2019

dscho Dec 15, 2019

dscho Dec 11, 2019

PhilipOakley Dec 13, 2019

dscho commented Dec 11, 2019

PhilipOakley commented Dec 13, 2019

dscho commented Dec 13, 2019

PhilipOakley commented Dec 13, 2019

dscho commented Dec 13, 2019

PhilipOakley commented Dec 14, 2019

dscho commented Dec 14, 2019

PhilipOakley commented Dec 14, 2019

PhilipOakley commented Dec 14, 2019

PhilipOakley commented Dec 14, 2019

dscho Dec 15, 2019

dscho Dec 15, 2019

dscho Dec 15, 2019

dscho Dec 15, 2019

dscho Dec 15, 2019

dscho commented Oct 6, 2020

Visual Studio solution: check for updates of vcpkg #2424

Visual Studio solution: check for updates of vcpkg #2424

Conversation

PhilipOakley commented Dec 8, 2019

PhilipOakley commented Dec 8, 2019

PhilipOakley commented Dec 8, 2019

dscho left a 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

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

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

Choose a reason for hiding this comment

dscho commented Dec 11, 2019

PhilipOakley commented Dec 13, 2019

dscho commented Dec 13, 2019

PhilipOakley commented Dec 13, 2019

dscho commented Dec 13, 2019

PhilipOakley commented Dec 14, 2019

dscho commented Dec 14, 2019

PhilipOakley commented Dec 14, 2019

PhilipOakley commented Dec 14, 2019

PhilipOakley commented Dec 14, 2019

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

dscho commented Oct 6, 2020