Small patch to fix build on Windows

Hi,

At Tue, 6 Aug 2019 22:50:14 +0300, Dmitry Igrishin <dmitigr@gmail.com> wrote in <CAAfz9KO4Nt-kDUKAcEKFND+1LeZ6nH_hjPGamonfTeZLRKz0bg@mail.gmail.com>

The attached self-documented patch fixes build on Windows in case when
path to Python has embedded spaces.

-          $solution->{options}->{python} . "\\python -c \"$pythonprog\"";
+          "\"$solution->{options}->{python}\\python\" -c \"$pythonprog\"";

Solution.pm has the following line:

my $opensslcmd =
$self->{options}->{openssl} . "\\bin\\openssl.exe version 2>&1";

AFAICS that's all.

-    if ($lib =~ m/\s/)
-    {
-        $lib = '"' . $lib . """;
-    }
+    # Since VC automatically quotes paths specified as the data of
+    # <AdditionalDependencies> in VC project file, it's mistakably
+    # to quote them here. Thus, it's okay if $lib contains spaces.

I'm not sure, but it's not likely that someone adds it without
actually stumbling on space-containing paths with the ealier
version. Anyway if we shouldn't touch this unless the existing
code makes actual problem.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

ср, 7 авг. 2019 г. в 11:29, Kyotaro Horiguchi <horikyota.ntt@gmail.com>:

Hi,

At Tue, 6 Aug 2019 22:50:14 +0300, Dmitry Igrishin <dmitigr@gmail.com> wrote in <CAAfz9KO4Nt-kDUKAcEKFND+1LeZ6nH_hjPGamonfTeZLRKz0bg@mail.gmail.com>

The attached self-documented patch fixes build on Windows in case when
path to Python has embedded spaces.

-          $solution->{options}->{python} . "\\python -c \"$pythonprog\"";
+          "\"$solution->{options}->{python}\\python\" -c \"$pythonprog\"";

Solution.pm has the following line:

my $opensslcmd =
$self->{options}->{openssl} . "\\bin\\openssl.exe version 2>&1";

AFAICS that's all.

Thank you! The attached 2nd version of the patch fixes this too.

-    if ($lib =~ m/\s/)
-    {
-        $lib = '"' . $lib . """;
-    }
+    # Since VC automatically quotes paths specified as the data of
+    # <AdditionalDependencies> in VC project file, it's mistakably
+    # to quote them here. Thus, it's okay if $lib contains spaces.

I'm not sure, but it's not likely that someone adds it without
actually stumbling on space-containing paths with the ealier
version. Anyway if we shouldn't touch this unless the existing
code makes actual problem.

So, do you think a comment is not needed here?

On Wed, Aug 7, 2019 at 11:11 AM Dmitry Igrishin <dmitigr@gmail.com> wrote:

ср, 7 авг. 2019 г. в 11:29, Kyotaro Horiguchi <horikyota.ntt@gmail.com>:

Solution.pm has the following line:

my $opensslcmd =
$self->{options}->{openssl} . "\\bin\\openssl.exe version 2>&1";

AFAICS that's all.

Thank you! The attached 2nd version of the patch fixes this too.

At some point the propossed patch for opensslcmd was like:

+ my $opensslprog = '\bin\openssl.exe version 2>&1';
+ my $opensslcmd = '"' . $self->{options}->{openssl} . '"' . $opensslprog;

It can be a question of taste, but I think the dot is easier to read.

Regards,

Juan José Santamaría Flecha

ср, 7 авг. 2019 г. в 15:33, Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com>:

On Wed, Aug 7, 2019 at 11:11 AM Dmitry Igrishin <dmitigr@gmail.com> wrote:

ср, 7 авг. 2019 г. в 11:29, Kyotaro Horiguchi <horikyota.ntt@gmail.com>:

Solution.pm has the following line:

my $opensslcmd =
$self->{options}->{openssl} . "\\bin\\openssl.exe version 2>&1";

AFAICS that's all.

Thank you! The attached 2nd version of the patch fixes this too.

At some point the propossed patch for opensslcmd was like:

+ my $opensslprog = '\bin\openssl.exe version 2>&1';
+ my $opensslcmd = '"' . $self->{options}->{openssl} . '"' . $opensslprog;

It can be a question of taste, but I think the dot is easier to read.

Well, the style inconsistent anyway, for example, in file Project.pm

$self->{def} = "./__CFGNAME__/$self->{name}/$self->{name}.def";
$self->{implib} = "__CFGNAME__/$self->{name}/$libname";

So, I don't know what style is preferable. Personally, I don't care.

Hello.

At Wed, 7 Aug 2019 12:14:48 +0300, Dmitry Igrishin <dmitigr@gmail.com> wrote in <CAAfz9KPVff92Np51DvvCDvqvxVchiuuvJCzz56qtM=N0SUnG8A@mail.gmail.com>

-    if ($lib =~ m/\s/)
-    {
-        $lib = '"' . $lib . """;
-    }
+    # Since VC automatically quotes paths specified as the data of
+    # <AdditionalDependencies> in VC project file, it's mistakably
+    # to quote them here. Thus, it's okay if $lib contains spaces.

I'm not sure, but it's not likely that someone adds it without
actually stumbling on space-containing paths with the ealier
version. Anyway if we shouldn't touch this unless the existing
code makes actual problem.

So, do you think a comment is not needed here?

# Sorry the last phrase above is broken.

I meant "if it ain't broke don't fix it".

I doubt that some older versions of VC might need it. I confirmed
that the extra &quot; actually harms at least VC2019 and the code
removal in the patch works. As for the replace comment, I'm not
sure it is needed since I think quoting is not the task for
AddLibrary/AddIncludeDir in the first place (and AddIncludeDir
doesn't have the same comment).

Now I'm trying to install VS2015 into my alomost-filled-up disk..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

чт, 8 авг. 2019 г. в 06:18, Kyotaro Horiguchi <horikyota.ntt@gmail.com>:

Hello.

At Wed, 7 Aug 2019 12:14:48 +0300, Dmitry Igrishin <dmitigr@gmail.com> wrote in <CAAfz9KPVff92Np51DvvCDvqvxVchiuuvJCzz56qtM=N0SUnG8A@mail.gmail.com>

-    if ($lib =~ m/\s/)
-    {
-        $lib = '"' . $lib . """;
-    }
+    # Since VC automatically quotes paths specified as the data of
+    # <AdditionalDependencies> in VC project file, it's mistakably
+    # to quote them here. Thus, it's okay if $lib contains spaces.

I'm not sure, but it's not likely that someone adds it without
actually stumbling on space-containing paths with the ealier
version. Anyway if we shouldn't touch this unless the existing
code makes actual problem.

So, do you think a comment is not needed here?

# Sorry the last phrase above is broken.

I meant "if it ain't broke don't fix it".

I doubt that some older versions of VC might need it. I confirmed
that the extra &quot; actually harms at least VC2019 and the code
removal in the patch works.

The code removal is required also to build on VC2017.

As for the replace comment, I'm not
sure it is needed since I think quoting is not the task for
AddLibrary/AddIncludeDir in the first place (and AddIncludeDir
doesn't have the same comment).

The attached 3rd version of the patch contains no comment in AddLibrary().

Now I'm trying to install VS2015 into my alomost-filled-up disk..

Thank you!

As for the replace comment, I'm not
sure it is needed since I think quoting is not the task for
AddLibrary/AddIncludeDir in the first place (and AddIncludeDir
doesn't have the same comment).

The attached 3rd version of the patch contains no comment in AddLibrary().

Sorry, forgot to attach the patch to the previous mail.

Hello.

At Thu, 08 Aug 2019 12:15:38 +0900 (Tokyo Standard Time), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in <20190808.121538.87367461.horikyota.ntt@gmail.com>

At Wed, 7 Aug 2019 12:14:48 +0300, Dmitry Igrishin <dmitigr@gmail.com> wrote in <CAAfz9KPVff92Np51DvvCDvqvxVchiuuvJCzz56qtM=N0SUnG8A@mail.gmail.com>

-    if ($lib =~ m/\s/)
-    {
-        $lib = '"' . $lib . """;
-    }
+    # Since VC automatically quotes paths specified as the data of
+    # <AdditionalDependencies> in VC project file, it's mistakably
+    # to quote them here. Thus, it's okay if $lib contains spaces.

..

I doubt that some older versions of VC might need it. I confirmed
that the extra &quot; actually harms at least VC2019 and the code
removal in the patch works. As for the replace comment, I'm not
sure it is needed since I think quoting is not the task for
AddLibrary/AddIncludeDir in the first place (and AddIncludeDir
doesn't have the same comment).

Now I'm trying to install VS2015 into my alomost-filled-up disk..

I confirmed that VC2015 works with the above diff. So the patch
is overall fine with me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2019-Aug-08, Dmitry Igrishin wrote:

my $prefixcmd =
-		  $solution->{options}->{python} . "\\python -c \"$pythonprog\"";
+		  "\"$solution->{options}->{python}\\python\" -c \"$pythonprog\"";

I think you can make this prettier like this:

my $prefixcmd = qq{"$solution->{options}->{python}\\python" -c "$pythonprog"};

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

чт, 8 авг. 2019 г. в 20:07, Alvaro Herrera <alvherre@2ndquadrant.com>:

On 2019-Aug-08, Dmitry Igrishin wrote:

my $prefixcmd =
-               $solution->{options}->{python} . "\\python -c \"$pythonprog\"";
+               "\"$solution->{options}->{python}\\python\" -c \"$pythonprog\"";

I think you can make this prettier like this:

my $prefixcmd = qq{"$solution->{options}->{python}\\python" -c "$pythonprog"};

This looks nice for a Perl hacker :-). As for me, it looks unusual and
a bit confusing. I never
programmed in Perl, but I was able to quickly understand where the
problem lies due to the
style adopted in other languages, when the contents are enclosed in
quotation marks, and
the quotation marks are escaped if they are part of the contents.
So, should I fix it? Any thoughts?

On Thu, Aug 08, 2019 at 10:46:07PM +0300, Dmitry Igrishin wrote:

This looks nice for a Perl hacker :-). As for me, it looks unusual and
a bit confusing. I never
programmed in Perl, but I was able to quickly understand where the
problem lies due to the
style adopted in other languages, when the contents are enclosed in
quotation marks, and
the quotation marks are escaped if they are part of the contents.
So, should I fix it? Any thoughts?

FWIW, I like Alvaro's suggestion about qq{} in this case, as it makes
sure that double-quotes are correctly applied where they should.
--
Michael

пт, 9 авг. 2019 г. в 05:45, Michael Paquier <michael@paquier.xyz>:

On Thu, Aug 08, 2019 at 10:46:07PM +0300, Dmitry Igrishin wrote:

This looks nice for a Perl hacker :-). As for me, it looks unusual and
a bit confusing. I never
programmed in Perl, but I was able to quickly understand where the
problem lies due to the
style adopted in other languages, when the contents are enclosed in
quotation marks, and
the quotation marks are escaped if they are part of the contents.
So, should I fix it? Any thoughts?

FWIW, I like Alvaro's suggestion about qq{} in this case, as it makes
sure that double-quotes are correctly applied where they should.

The attached 4rd version of the patch uses qq||. I used qq|| instead
of qq{} for consistency because qq|| is already used in Solution.pm:

return qq|VisualStudioVersion = $self->{VisualStudioVersion}
MinimumVisualStudioVersion = $self->{MinimumVisualStudioVersion}
|;

At Fri, 9 Aug 2019 09:56:27 +0300, Dmitry Igrishin <dmitigr@gmail.com> wrote in <CAAfz9KPZbPjoWTqOb5moi_YWvdbSjAMZsrVBW0cBw33Q560CLw@mail.gmail.com>

пт, 9 авг. 2019 г. в 05:45, Michael Paquier <michael@paquier.xyz>:

On Thu, Aug 08, 2019 at 10:46:07PM +0300, Dmitry Igrishin wrote:

This looks nice for a Perl hacker :-). As for me, it looks unusual and
a bit confusing. I never
programmed in Perl, but I was able to quickly understand where the
problem lies due to the
style adopted in other languages, when the contents are enclosed in
quotation marks, and
the quotation marks are escaped if they are part of the contents.
So, should I fix it? Any thoughts?

FWIW, I like Alvaro's suggestion about qq{} in this case, as it makes
sure that double-quotes are correctly applied where they should.

The attached 4rd version of the patch uses qq||. I used qq|| instead
of qq{} for consistency because qq|| is already used in Solution.pm:

return qq|VisualStudioVersion = $self->{VisualStudioVersion}
MinimumVisualStudioVersion = $self->{MinimumVisualStudioVersion}
|;

Hmm. qq is nice but '|' make my eyes twitch (a bit). Couldn't we
use other delimites like (), ##, or // ? (I like {} for use in
this patch.)

Any opinions?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

пт, 9 авг. 2019 г. в 10:23, Kyotaro Horiguchi <horikyota.ntt@gmail.com>:

At Fri, 9 Aug 2019 09:56:27 +0300, Dmitry Igrishin <dmitigr@gmail.com> wrote in <CAAfz9KPZbPjoWTqOb5moi_YWvdbSjAMZsrVBW0cBw33Q560CLw@mail.gmail.com>

пт, 9 авг. 2019 г. в 05:45, Michael Paquier <michael@paquier.xyz>:

On Thu, Aug 08, 2019 at 10:46:07PM +0300, Dmitry Igrishin wrote:

This looks nice for a Perl hacker :-). As for me, it looks unusual and
a bit confusing. I never
programmed in Perl, but I was able to quickly understand where the
problem lies due to the
style adopted in other languages, when the contents are enclosed in
quotation marks, and
the quotation marks are escaped if they are part of the contents.
So, should I fix it? Any thoughts?

FWIW, I like Alvaro's suggestion about qq{} in this case, as it makes
sure that double-quotes are correctly applied where they should.

The attached 4rd version of the patch uses qq||. I used qq|| instead
of qq{} for consistency because qq|| is already used in Solution.pm:

return qq|VisualStudioVersion = $self->{VisualStudioVersion}
MinimumVisualStudioVersion = $self->{MinimumVisualStudioVersion}
|;

Hmm. qq is nice but '|' make my eyes twitch (a bit). Couldn't we
use other delimites like (), ##, or // ? (I like {} for use in
this patch.)

Any opinions?

Personally I don't care. I used || notation only in order to be
consistent, since this notation is already used in Solution.pm. If
this consistency is not required let me provide a patch with {}
notation. What do you think?

On Fri, Aug 09, 2019 at 11:21:52AM +0300, Dmitry Igrishin wrote:

Personally I don't care. I used || notation only in order to be
consistent, since this notation is already used in Solution.pm. If
this consistency is not required let me provide a patch with {}
notation. What do you think?

We are talking about one place in src/tools/msvc/ using qq on HEAD.
So one or the other is fine by me as long as we remain in the
acceptable ASCII ranks.
--
Michael

вт, 13 авг. 2019 г. в 06:19, Michael Paquier <michael@paquier.xyz>:

On Fri, Aug 09, 2019 at 11:21:52AM +0300, Dmitry Igrishin wrote:

Personally I don't care. I used || notation only in order to be
consistent, since this notation is already used in Solution.pm. If
this consistency is not required let me provide a patch with {}
notation. What do you think?

We are talking about one place in src/tools/msvc/ using qq on HEAD.
So one or the other is fine by me as long as we remain in the
acceptable ASCII ranks.

Okay. 5th version of patch is attached.