Fix of #4665 to have a simple file based check for the VC++ 2015 redistributables by bergmeister · Pull Request #4745 · PowerShell/PowerShell

bergmeister · Sep 3, 2017

To fix #4665
The previous check registry check of SOFTWARE\Microsoft\DevDiv\VC\Servicing\14.0\RuntimeMinimum turned out to not work in the scenario whereby the redistributables were installed via Windows update (which does not use MSI) because the registry entry did not get populated in this case.
In the issue discussion @SteveL-MSFT suggested to have only a simple file based check, therefore this PR simply checks for the existence of vcruntime140.dll in the Windows/System32 folder

The 'Pending' attribute was removed from existing tests since the download links are now present again and the tests were improved using Pester TestCases.

…015 redistributables. Note that this is specific to 2015 (vcruntime140.dll refers to the Visual Studio version 14.0, which maps to Visual Studio 2015). A previous check registry check of 'SOFTWARE\Microsoft\DevDiv\VC\Servicing\14.0\RuntimeMinimum' failed because when the redistributables are installed via Windows update (which does not use MSI), then the registry entry did not get populated. The 'Pending' attribute was removed from existing tests since the download links are now present again and the tests were improved using Pester TestCases.

msftclas · Sep 3, 2017

@bergmeister,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

SteveL-MSFT · Sep 4, 2017

+    <!-- Prerequisite check for Windows Universal C time -->
    <Property Id="UCRTINSTALLED" Secure="yes">
-      <DirectorySearch Id="Windows_System32" Path="[WindowsFolder]System32" Depth="0">
+      <DirectorySearch Id="Windows_System32_search1" Path="[WindowsFolder]System32" Depth="0">


Would it be better to call this Universal_Runtime_Search?

I can make 'search' uppercase but the numbers at the end are needed now because otherwise WiX gives a compilation error that the DirectorySearch Id is duplicated. This seems to be a WiX peculiarity in the case of file search properties and I have seen other people do the same as well but if you know a better way of doing it, feel free to propose.

I see. Not a blocking issue for me.

Sorry, I see only now that I misread your 2 comment value suggestions as Windows_System32_Search, which would have resulted in the duplicated DirectorySearch Id compilation error (which I actually had in the first place when I just copy pasted the existing block as a template with some modifications). So, yes, you could name it Universal_Runtime_Search but I am not sure if there is much value because the property name already contains this information and this Id is not used anywhere (but needed for compilation). Maybe it is better to rename the property value(s) instead?

My thinking is that if we need to extend this in the future, Windows_System32_searchN doesn't really provide much information. If the requirement is simply it has to be unique, I think something more descriptive to what the search does is preferred.

OK, I did more research and found a documented solution, which allows to define it once and then reference it in order to get rid of this problem. Please have a look at the new commit.

SteveL-MSFT · Sep 4, 2017


+     <!-- Prerequisite check for Visual Studio 2015 C++ redistributables -->
+     <Property Id="VCRUNTIMEINSTALLED" Secure="yes">
+      <DirectorySearch Id="Windows_System32_search2" Path="[WindowsFolder]System32" Depth="0">


I think it's better to calls this Visual_CPP_Runtime_Search

iSazonov · Sep 4, 2017

-	
-    <!-- Prerequisites check for Windows Universal C time and Visual Studio 2015 C++ redistributables -->
+
+    <!-- Prerequisite check for Windows Universal C time -->


Typo time -> runtime.

Thanks. I fixed it.

iSazonov · Sep 4, 2017

+        <FileSearch Name="vcruntime140.dll"/>
+      </DirectorySearch>
+    </Property>	
+    <Condition Message="$(env.ProductName) requires the Visual Studio 2015 C++ redistributables to be installed. You can download it here: https://www.microsoft.com/download/details.aspx?id=48145">


The link is not persistent and may be changed in any time. I think it is better remove it. Anybody can find it by means of Bing/Google.

This is why there are tests to check that the links are still active. Also the original issue #4458 requested that it 'should give a good error message directing user to download'

I don't know how often those links get recycled, but it seems another solution would be to link back to our pre-requisites page: https://github.com/PowerShell/PowerShell/blob/master/docs/installation/windows.md#prerequisites

Ok. Do you want me to link to the pre-requisites page then or do you want some time to think about it first? If those links get recycled then the pre-requisites page will suffer from the same issue (except for that it can be updated even after release, which is not an insignificant advantage)

@bergmeister the fact that we can update the pre-reqs page fairly easily and quickly is the advantage I was thinking

@SteveL-MSFT Can PG request a permanent link for Universal C runtime?

…cing according to this special trick in the official WiX documentation here: http://wixtoolset.org/documentation/manual/v3/howtos/files_and_registry/directorysearchref.html Renamed property values as suggested.

SteveL-MSFT · Sep 4, 2017

-}
+    It "Download link '<downloadLink>' for '<Name>' is reachable" -TestCases $downloadLinks -Test {
+        Param ([string]$downloadLink)
+        (Invoke-WebRequest $universalCRuntimeDownloadLink.Replace("https://", 'http://')) | Should Not Be $null


Why are you replacing HTTPS with HTTP?

Also, perhaps if we just want to validate the download links haven't been removed, we should just check that you get a 200 status?

The replacement of https with http was because I sometimes got errors, I can take it out if you don't like it.
If you take an invalid URL like e.g. https://www.microsoft.com/download/details.aspx?id=5041000000, then you still get a 200 response because I get redirected to an error page but the returned object is $null

I think it's better to leave as HTTPS. Depending on why it's failing, you could try it a few times before failing the test since this is Scenario.

For the $null vs 200 status, you should add a comment so other people understand why you made the choice.

… Use -UseBasicParsing switch with the hope of not having failures in the CI environment. Added comment why there is no assertion about the StatusCode.

…in PR 4745 because this page is easier to update

bergmeister · Sep 6, 2017

Please do not merge. My last commit, which replaced the Urls had a WiX compilation error (maybe due to the hash in the url). The Appveyor build should be improved to go red if it does not produce an MSI.

SteveL-MSFT · Sep 6, 2017

@bergmeister can you open an issue to test the MSI packaging?

…rs for file search property Ids because due to them being used for a search property means that they must be public, hence lowercase is not allowed.

bergmeister · Sep 6, 2017

@SteveL-MSFT I opened issue #4760 for it and fixed the compilation error (Property Id had to be public due to the DirectorySearch and therefore did not allow lowercase characters). I installed the MSI produced by the build on my machine as a test and it would be ready for merge again.

mirichmo · Sep 9, 2017

I restarted the Travis build to see if it would fix itself.

msftclas added the cla-already-signed label Sep 3, 2017

bergmeister mentioned this pull request Sep 3, 2017

WindowsInstaller.Tests.ps1 failed on windows #4720

Closed

SteveL-MSFT reviewed Sep 4, 2017

View reviewed changes

iSazonov reviewed Sep 4, 2017

View reviewed changes

iSazonov assigned mirichmo Sep 4, 2017

iSazonov requested a review from mirichmo September 4, 2017 03:51

bergmeister added 2 commits September 4, 2017 18:46

Fixed typo spotted in code review of PR 4745

087da6a

Remove DirectorySearch Id duplication by defining it once and referen…

6a37fd5

…cing according to this special trick in the official WiX documentation here: http://wixtoolset.org/documentation/manual/v3/howtos/files_and_registry/directorysearchref.html Renamed property values as suggested.

SteveL-MSFT requested changes Sep 4, 2017

View reviewed changes

Remove replacements of HTTPS with HTTP, i.e. test against exact link.…

3c6bd28

… Use -UseBasicParsing switch with the hope of not having failures in the CI environment. Added comment why there is no assertion about the StatusCode.

SteveL-MSFT approved these changes Sep 5, 2017

View reviewed changes

Replace download links with link to pre-requisites page as suggested …

af9d464

…in PR 4745 because this page is easier to update

Compilation error CNDL0012 fix: WiX does not allow lowercase characte…

e137ae9

…rs for file search property Ids because due to them being used for a search property means that they must be public, hence lowercase is not allowed.

mirichmo approved these changes Sep 9, 2017

View reviewed changes

bergmeister mentioned this pull request Sep 12, 2017

Test failed "Should have download link about Universal C runtime that is reachable" #4804

Closed

mirichmo merged commit da49841 into PowerShell:master Sep 15, 2017

SteveL-MSFT mentioned this pull request Oct 6, 2017

Windows MSI beta.8 pkg on Windows 10 requires VC++ runtime redist #5029

Closed


		<!-- Prerequisites check for Windows Universal C time and Visual Studio 2015 C++ redistributables -->

		<!-- Prerequisite check for Windows Universal C time -->

Search code, repositories, users, issues, pull requests...

Conversation

bergmeister commented Sep 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Sep 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister commented Sep 6, 2017

Uh oh!

SteveL-MSFT commented Sep 6, 2017

Uh oh!

bergmeister commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirichmo commented Sep 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bergmeister commented Sep 3, 2017 •

edited

Loading

bergmeister Sep 4, 2017 •

edited

Loading

bergmeister Sep 4, 2017 •

edited

Loading

bergmeister commented Sep 6, 2017 •

edited

Loading