On Thu, 27 Jun 2019 at 17:28, Michael Paquier <mich...@paquier.xyz> wrote:
> 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.
>
Thanks for the review.
> - <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?
>
We stopped the support of building with all the visual studio versions less
than 2013.
I updated the SDK versions accordingly.
> -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.
>
The change in the patch is good.
>
> - 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?
>
Yes, that is a good change, I thought of doing the same, but I don't know
how to do it.
The similar change is required for the CreateProject also.
> + # 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.
>
The change is good, but the comment is wrong.
+ # The major visual studio that is supported has nmake
+ # version >= 14.30, so stick with it as the latest version
The major visual studio version that is supported has nmake version <=14.30
Except for the above two changes, overall the patch is in good shape.
Regards,
Haribabu Kommi
