Обсуждение: Small patch to fix build on Windows
Hi, The attached self-documented patch fixes build on Windows in case when path to Python has embedded spaces.
Вложения
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 " 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 " 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 " 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.