On Wed, Jun 26, 2019 at 10:29:05PM +1000, Haribabu Kommi wrote:
> Thanks for the review. Yes, that patch applies till 9.5, it is my mistake
> in naming the patch.
I have been able to finally set up an environment with VS 2019 (as
usual this stuff needs time, anyway..), and I can confirm that the
patch is able to compile properly.
- <productname>Visual Studio 2017</productname> (including Express editions),
- as well as standalone Windows SDK releases 6.0 to 8.1.
+ <productname>Visual Studio 2019</productname> (including Express editions),
+ as well as standalone Windows SDK releases 8.1a to 10.
I would like to understand why this range of requirements is updated.
Is there any reason to do so. If we change these docs, what does it
mean in terms of versions of Visual Studio supported?
-or a VS2015Solution or a VS2017Solution, all in Solution.pm, depending on
-the user's build environment) and adding objects implementing the corresponding
-Project interface (VC2013Project or VC2015Project or VC2017Project from
-MSBuildProject.pm) to it.
+or a VS2015Solution or a VS2017Solution or a VS2019Solution, all in Solution.pm,
+depending on the user's build environment) and adding objects implementing
+the corresponding Project interface (VC2013Project or VC2015Project or VC2017Project
+or VC2019Project from MSBuildProject.pm) to it.
This formulation is weird the more we accumulate new objects, let's
put that in a proper list of elements separated with commas except
for the two last ones which should use "or".
s/greather/greater/.
The patch still has typos, and the format is not satisfying yet, so I
have done a set of fixes as per the attached.
- elsif ($major < 6)
+ elsif ($major < 12)
{
croak
- "Unable to determine Visual Studio version:
Visual Studio versions before 6.0 aren't supported.";
+ "Unable to determine Visual Studio version:
Visual Studio versions before 12.0 aren't supported.";
Well, this is a separate bug fix, still I don't mind fixing that in
the same patch as we meddle with those code paths now. Good catch.
- croak $visualStudioVersion;
+ carp $visualStudioVersion;
Same here. Just wouldn't it be better to print the version found in
the same message?
+ # The major visual studio that is supported has nmake version >=
14.20 and < 15.
if ($major > 14)
Comment line is too long. It seems to me that the condition here
should be ($major >= 14 && $minor >= 30). That's not completely
correct either as we have a version higher than 14.20 for VS 2019 but
that's better than just using 14.29 or a fake number I guess.
So for now I have the attached which applies to HEAD. The patch is
not indented yet because the conditions in CreateProject() and
CreateSolution() get messed up, but I'll figure out something.
Any comments? I am wondering the update related to the version range
of the standalone SDKs though. No need for backpatched versions yet,
first let's agree about the shape of what we want on HEAD.
--
Michael