Small patch to fix build on Windows

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Small patch to fix build on Windows

Dmitry Igrishin
Hi,

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

fix-mkvcbuild.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Kyotaro Horiguchi-4
Hi,

At Tue, 6 Aug 2019 22:50:14 +0300, Dmitry Igrishin <[hidden email]> wrote in <[hidden email]>
> 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 = '&quot;' . $lib . "&quot;";
-    }
+    # 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


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Dmitry Igrishin
ср, 7 авг. 2019 г. в 11:29, Kyotaro Horiguchi <[hidden email]>:

>
> Hi,
>
> At Tue, 6 Aug 2019 22:50:14 +0300, Dmitry Igrishin <[hidden email]> wrote in <[hidden email]>
> > 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 = '&quot;' . $lib . "&quot;";
> -    }
> +    # 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?

fix-mkvcbuild-2.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Juan José Santamaría Flecha
On Wed, Aug 7, 2019 at 11:11 AM Dmitry Igrishin <[hidden email]> wrote:

>
> ср, 7 авг. 2019 г. в 11:29, Kyotaro Horiguchi <[hidden email]>:
> >
> > 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


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Dmitry Igrishin
ср, 7 авг. 2019 г. в 15:33, Juan José Santamaría Flecha
<[hidden email]>:

>
> On Wed, Aug 7, 2019 at 11:11 AM Dmitry Igrishin <[hidden email]> wrote:
> >
> > ср, 7 авг. 2019 г. в 11:29, Kyotaro Horiguchi <[hidden email]>:
> > >
> > > 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.


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Kyotaro Horiguchi-4
In reply to this post by Dmitry Igrishin
Hello.

At Wed, 7 Aug 2019 12:14:48 +0300, Dmitry Igrishin <[hidden email]> wrote in <CAAfz9KPVff92Np51DvvCDvqvxVchiuuvJCzz56qtM=[hidden email]>

> > -    if ($lib =~ m/\s/)
> > -    {
> > -        $lib = '&quot;' . $lib . "&quot;";
> > -    }
> > +    # 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


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Dmitry Igrishin
чт, 8 авг. 2019 г. в 06:18, Kyotaro Horiguchi <[hidden email]>:

>
> Hello.
>
> At Wed, 7 Aug 2019 12:14:48 +0300, Dmitry Igrishin <[hidden email]> wrote in <CAAfz9KPVff92Np51DvvCDvqvxVchiuuvJCzz56qtM=[hidden email]>
> > > -    if ($lib =~ m/\s/)
> > > -    {
> > > -        $lib = '&quot;' . $lib . "&quot;";
> > > -    }
> > > +    # 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!


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Dmitry Igrishin
> > 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.

fix-mkvcbuild-3.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Kyotaro Horiguchi-4
In reply to this post by Kyotaro Horiguchi-4
Hello.

At Thu, 08 Aug 2019 12:15:38 +0900 (Tokyo Standard Time), Kyotaro Horiguchi <[hidden email]> wrote in <[hidden email]>
> At Wed, 7 Aug 2019 12:14:48 +0300, Dmitry Igrishin <[hidden email]> wrote in <CAAfz9KPVff92Np51DvvCDvqvxVchiuuvJCzz56qtM=[hidden email]>
> > > -    if ($lib =~ m/\s/)
> > > -    {
> > > -        $lib = '&quot;' . $lib . "&quot;";
> > > -    }
> > > +    # 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


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Alvaro Herrera-9
In reply to this post by Dmitry Igrishin
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


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Dmitry Igrishin
чт, 8 авг. 2019 г. в 20:07, Alvaro Herrera <[hidden email]>:

>
> 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?


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Dmitry Igrishin
пт, 9 авг. 2019 г. в 05:45, Michael Paquier <[hidden email]>:

>
> 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}
  |;

fix-mkvcbuild-4.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Kyotaro Horiguchi-4
At Fri, 9 Aug 2019 09:56:27 +0300, Dmitry Igrishin <[hidden email]> wrote in <[hidden email]>

> пт, 9 авг. 2019 г. в 05:45, Michael Paquier <[hidden email]>:
> >
> > 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


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Dmitry Igrishin
пт, 9 авг. 2019 г. в 10:23, Kyotaro Horiguchi <[hidden email]>:

>
> At Fri, 9 Aug 2019 09:56:27 +0300, Dmitry Igrishin <[hidden email]> wrote in <[hidden email]>
> > пт, 9 авг. 2019 г. в 05:45, Michael Paquier <[hidden email]>:
> > >
> > > 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?


Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Small patch to fix build on Windows

Dmitry Igrishin
вт, 13 авг. 2019 г. в 06:19, Michael Paquier <[hidden email]>:

>
> 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.

fix-mkvcbuild-5.patch (2K) Download Attachment