Thread: Compile from source using latest Microsoft Windows SDK

Hi Peifeng,

On Fri, Mar 29, 2019 at 12:01:26AM +0900, Peifeng Qiu wrote:
> The current Windows build system supports compiling with Windows SDK up to
> v8.1. When building with the latest Windows SDK v10 which is the default
> for Visual Studio 2017, we will get the following error:
>
> error MSB8036: The Windows SDK version 8.1 was not found.

Actually up to 10, no?  Sorry for the delay, I have just noticed this
patch registered in the commit fest.  And now is review time.

> When the build system generates projects files for MSBuild to consume, it
> doesn't include a SDK version number. Then MSBuild will assume v8.1 as
> default.
> But if we only install the latest v10 but not v8.1, MSBuild will error out.

So...  This actually boils down to that behavior:

https://developercommunity.visualstudio.com/content/problem/140294/windowstargetplatformversion-makes-it-impossible-t.html

While WindowsSDKVersion seems to be present all the time.  I think
that we should be more defensive if the variable is not defined, and
instead rely on the default provided by the system, whatever it may
be.  In short it seems to me that the tag WindowsTargetPlatformVersion
should be added only if the variable exists, and your patch always
sets it.

For anything with Postgres on Windows, I have been using Visual Studio
2015 and 2019 lately to compile Postgres mainly with the Native Tools
command prompt so I have never actually faced this failure even with
the most recent VS 2019.  Using just a command prompt causes a failure
when finding out nmake for example as that's not in the default PATH.
Our buildfarm members don't complain either, and there are two animals
using VS 2017: hamerkop and bowerbird.
--
Michael

On Fri, Jul 19, 2019 at 03:39:49PM +0800, Peifeng Qiu wrote:
> I updated the patch to only include the WindowsTargetPlatformVersion node
> if WindowsSDKVersion is present.  I can confirm that this issue no
> longer exists for VS2019. So only VS2017 is problematic.

(Could you please avoid to top-post?)
Ugh.  That's one I don't have at hand.

> I'm also very curious on how hamerkop and bowerbird build postgres with
> VS2017.  Looks like hamerkop and bowerbird both exist before VS2017
> and maybe they get SDK v8.1 from previous VS installations. I will
> contact admin of hamerkop and bowerbird and see if that's the case.
> As of now I can still encounter the same issue with fresh installed
> Windows Server 2016 and VS2017, on both azure and google cloud. So
> better to patch the build system anyway.

I guess so but I cannot confirm myself.  I am adding Andrew and Hari
in CC to double-check if they have seen this problem or not.  Hari has
also worked on porting VS 2017 and 2019 in the MSVC scripts in the
tree.
--
Michael

On 7/19/19 5:51 AM, Michael Paquier wrote:
>
>> I'm also very curious on how hamerkop and bowerbird build postgres with
>> VS2017.  Looks like hamerkop and bowerbird both exist before VS2017
>> and maybe they get SDK v8.1 from previous VS installations. I will
>> contact admin of hamerkop and bowerbird and see if that's the case.
>> As of now I can still encounter the same issue with fresh installed
>> Windows Server 2016 and VS2017, on both azure and google cloud. So
>> better to patch the build system anyway.
> I guess so but I cannot confirm myself.  I am adding Andrew and Hari
> in CC to double-check if they have seen this problem or not.  Hari has
> also worked on porting VS 2017 and 2019 in the MSVC scripts in the
> tree.



My tests of the VS2017 stuff used this install mechanism on a fresh
Windows instance:


choco install -y visualstudio2017-workload-vctools --package-parameters
"--includeOptional"


This installed Windows Kits 8.1 and 10, among many other things.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jul 19, 2019 at 08:30:38AM -0400, Andrew Dunstan wrote:
> My tests of the VS2017 stuff used this install mechanism on a fresh
> Windows instance:
>
> choco install -y visualstudio2017-workload-vctools --package-parameters
> "--includeOptional"
>
> This installed Windows Kits 8.1 and 10, among many other things.

So you have bypassed the problem by installing the v8.1 SDK.  And if
you don't do that and only include the v10 SDK, then you see the
problem.  Functionally, it also means that with a VS2017 compilation
the SDK version is forcibly downgraded, isn't that a bad idea anyway?
--
Michael

On 7/19/19 9:10 PM, Michael Paquier wrote:
> On Fri, Jul 19, 2019 at 08:30:38AM -0400, Andrew Dunstan wrote:
>> My tests of the VS2017 stuff used this install mechanism on a fresh
>> Windows instance:
>>
>> choco install -y visualstudio2017-workload-vctools --package-parameters
>> "--includeOptional"
>>
>> This installed Windows Kits 8.1 and 10, among many other things.
> So you have bypassed the problem by installing the v8.1 SDK.  And if
> you don't do that and only include the v10 SDK, then you see the
> problem.  Functionally, it also means that with a VS2017 compilation
> the SDK version is forcibly downgraded, isn't that a bad idea anyway?



For VS2017, the 8.1 SDK is part of the optional package set (see

<https://docs.microsoft.com/en-us/visualstudio/install/workload-component-id-vs-build-tools?view=vs-2017#visual-c-build-tools>)
but for VS2019 it is not (see

<https://docs.microsoft.com/en-us/visualstudio/install/workload-component-id-vs-build-tools?view=vs-2019#visual-c-build-tools>)
so yes, we need to deal with the issue, but it's really only a major
issue for VS2019, ISTM. I guess we might need a test for what SDK is
available? That's going to be fun ...


cheers


andrew

--
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jul 22, 2019 at 04:01:46PM +0800, Peifeng Qiu wrote:
>> but it's really only a major issue for VS2019
>
> VS2019 will use the latest v10 SDK by default. So no need to install 8.1
> for VS2019.

Yes, FWIW, I have tested with VS2019 when committing 2b1394f, and in
this case only the v10 SDK got installed, with no actual issues
related to the dependency of the SDK reported.  In this case I have
installed VS using the community installer provided by Microsoft.

>> I guess we might need a test for what SDK is available?
>
> We can just use the WindowsSDKVersion environment variable to
> determine the SDK for current cmd session. It's set when you start
> the Visual Studio Prompt or call one bat script.  Developers can
> choose the right version best suit their need. Detecting all
> installed SDK version can be done with some registry magic but I
> think that's not necessary in this case.

This looks more sensible to do if the environment variable is
available.  Looking around this variable is available when using the
command prompt for native tools.  So using it sounds like a good idea
to me if it exists.
--
Michael

On 7/22/19 4:23 AM, Michael Paquier wrote:
> On Mon, Jul 22, 2019 at 04:01:46PM +0800, Peifeng Qiu wrote:
>>> but it's really only a major issue for VS2019
>> VS2019 will use the latest v10 SDK by default. So no need to install 8.1
>> for VS2019.
> Yes, FWIW, I have tested with VS2019 when committing 2b1394f, and in
> this case only the v10 SDK got installed, with no actual issues
> related to the dependency of the SDK reported.  In this case I have
> installed VS using the community installer provided by Microsoft.
>
>>> I guess we might need a test for what SDK is available?
>> We can just use the WindowsSDKVersion environment variable to
>> determine the SDK for current cmd session. It's set when you start
>> the Visual Studio Prompt or call one bat script.  Developers can
>> choose the right version best suit their need. Detecting all
>> installed SDK version can be done with some registry magic but I
>> think that's not necessary in this case.
> This looks more sensible to do if the environment variable is
> available.  Looking around this variable is available when using the
> command prompt for native tools.  So using it sounds like a good idea
> to me if it exists.



Yeah, on consideration I think Peifeng's patch upthread looks OK.
(Incidentally, this variable is not set in the very old version of VC
running on currawong).


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jul 24, 2019 at 10:38:47AM -0400, Andrew Dunstan wrote:
> Yeah, on consideration I think Peifeng's patch upthread looks OK.
> (Incidentally, this variable is not set in the very old version of VC
> running on currawong).

Interesting.  I am not actually sure in which version of VS this has
been introduced.  But it would be fine enough to do nothing if the
variable is not defined and rely on the default.  Except for the
formatting and indentation, the patch looks right.  Andrew, perhaps
you would prefer doing the final touch on it and commit it?
--
Michael

On Thu, Jul 25, 2019 at 09:02:14AM +0900, Michael Paquier wrote:
> Interesting.  I am not actually sure in which version of VS this has
> been introduced.  But it would be fine enough to do nothing if the
> variable is not defined and rely on the default.  Except for the
> formatting and indentation, the patch looks right.  Andrew, perhaps
> you would prefer doing the final touch on it and commit it?

Andrew has just applied the patch as of 20e99cd (+ cb9bb15 for an
extra fix by Tom).
--
Michael