Thread: Postgres Windows build system doesn't work with python installed inProgram Files

Collegues,

Accidently I've come over minor bug in the Mkvcbuild.pm.
It happens, that it doesn't tolerate spaces in the $config->{python}
path, because it want to call python in order to find out version,
prefix and so on, and doesn't properly quote command.

Fix is very simple, see attach.

Patch is made against REL_12_STABLE, but probably applicable to other
versions as well.
--

On Thu, Apr 30, 2020 at 03:06:08PM +0300, Victor Wagner wrote:
> Fix is very simple, see attach.
>
> Patch is made against REL_12_STABLE, but probably applicable to other
> versions as well.

Indeed, thanks.

>          my $pythonprog = "import sys;print(sys.prefix);"
>            . "print(str(sys.version_info[0])+str(sys.version_info[1]))";
>          my $prefixcmd =
> -          $solution->{options}->{python} . "\\python -c \"$pythonprog\"";
> +        '"' . $solution->{options}->{python} . "\\python\" -c \"$pythonprog\"";
>          my $pyout = `$prefixcmd`;
>          die "Could not query for python version!\n" if $?;
>          my ($pyprefix, $pyver) = split(/\r?\n/, $pyout);

This reminds me of ad7595b.  Wouldn't it be better to use qq() here?
--
Michael

В Fri, 1 May 2020 17:52:15 +0900
Michael Paquier <michael@paquier.xyz> пишет:

> On Thu, Apr 30, 2020 at 03:06:08PM +0300, Victor Wagner wrote:
> > Fix is very simple, see attach.
> >
> > Patch is made against REL_12_STABLE, but probably applicable to
> > other versions as well.
>
> Indeed, thanks.
>
> >          my $pythonprog = "import sys;print(sys.prefix);"
> >            .
> > "print(str(sys.version_info[0])+str(sys.version_info[1]))"; my
> > $prefixcmd =
> > -          $solution->{options}->{python} . "\\python -c
> > \"$pythonprog\"";
> > +        '"' . $solution->{options}->{python} . "\\python\"
> > -c \"$pythonprog\""; my $pyout = `$prefixcmd`;
> >          die "Could not query for python version!\n" if $?;
> >          my ($pyprefix, $pyver) = split(/\r?\n/, $pyout);
>
> This reminds me of ad7595b.  Wouldn't it be better to use qq() here?

Maybe. But probably original author of this code was afraid of using
too long chain of ->{} in the string substitution.

So, I left this style n place.

Nonetheless, using qq wouldn't save us from doubling backslashes.

--

On Sun, May 03, 2020 at 04:23:24PM -0300, Ranier Vilela wrote:
> I don't know if it applies to the same case, but from the moment I
> installed python on the development machine, the Postgres build stopped
> working correctly.
> Although perl, flex and bison are available in the path, the build does not
> generate files that depend on flex and bison.

Are you following the instructions of the documentation?  Here is a
link to them:
https://www.postgresql.org/docs/devel/install-windows-full.html

My guess is that you would be just missing a PATH configuration or
such because python enforced a new setting?

> Warning from build.pl
> Use of uninitialized value $ARGV[0] in uc at build.pl line 44.
> Use of uninitialized value $ARGV[0] in uc at build.pl line 48.

Hmm.  We have buildfarm machines using the MSVC scripts and Python,
see for example woodloose.  And note that @ARGV would be normally
defined, so your warning looks fishy to me.
--
Michael

On Mon, May 04, 2020 at 09:45:54AM +0200, Juan José Santamaría Flecha wrote:
> I think these are two different issues, python PATH and build.pl warnings.
> For the later, you can check woodloose logs and see the warning after
> commit 8f00d84afc.

Oh, indeed.  I somewhat managed to miss these in the logs of the
buildfarm.  What if we refactored the code of build.pl so as we'd
check first if $ARGV[0] is defined or not?  If not defined, then we
need to have a release-quality build for all the components.  How does
that sound?  Something not documented is that using "release" as first
argument enforces also a release-quality build for all the components,
so we had better not break that part.
--
Michael

On Fri, May 01, 2020 at 12:48:17PM +0300, Victor Wagner wrote:
> Maybe. But probably original author of this code was afraid of using
> too long chain of ->{} in the string substitution.
>
> So, I left this style n place.
>
> Nonetheless, using qq wouldn't save us from doubling backslashes.

Looking at this part in more details, I find the attached much more
readable.  I have been able to test it on my own Windows environment
and the problem gets fixed (I have reproduced the original problem as
well).
--
Michael

В Tue, 5 May 2020 15:45:48 +0900
Michael Paquier <michael@paquier.xyz> пишет:

> On Fri, May 01, 2020 at 12:48:17PM +0300, Victor Wagner wrote:
> > Maybe. But probably original author of this code was afraid of using
> > too long chain of ->{} in the string substitution.
> >
> > So, I left this style n place.
> >
> > Nonetheless, using qq wouldn't save us from doubling backslashes.
>
> Looking at this part in more details, I find the attached much more
> readable.  I have been able to test it on my own Windows environment

I agree, it is better.


> and the problem gets fixed (I have reproduced the original problem as
> well).
--

On Mon, May 04, 2020 at 03:42:20PM +0200, Juan José Santamaría Flecha wrote:
> +1, seems like the way to go to me.

Attached is a patch for that and I have gone with a simple solution,
with some bonus comments about the way things happen.  Here are the
patterns I tested for build.pl and the commands it generates, making
sure that we have the same commands with HEAD and the patch:
1) perl build.pl
msbuild pgsql.sln /verbosity:normal  /p:Configuration=Release
2) perl build.pl debug
msbuild pgsql.sln /verbosity:normal  /p:Configuration=Debug
3) perl build.pl release
msbuild pgsql.sln /verbosity:normal  /p:Configuration=Release
4) perl build.pl foo
msbuild foo.vcxproj /verbosity:normal  /p:Configuration=Release
5) perl build.pl debug foo
msbuild foo.vcxproj /verbosity:normal  /p:Configuration=Debug
6) perl build.pl release foo
msbuild foo.vcxproj /verbosity:normal  /p:Configuration=Release

The two warnings show up only in the first case, of course.
--
Michael

On Tue, May 05, 2020 at 10:16:23AM +0300, Victor Wagner wrote:
> I agree, it is better.

Thanks, applied and back-patched down to 9.5.  Now for the second
problem of this thread..
--
Michael

В Wed, 6 May 2020 10:21:41 -0300
Ranier Vilela <ranier.vf@gmail.com> пишет:

> Em qua., 6 de mai. de 2020 às 09:53, Michael Paquier
> <michael@paquier.xyz> escreveu:
>
> > On Tue, May 05, 2020 at 10:16:23AM +0300, Victor Wagner wrote:
> > > I agree, it is better.
> >
> > Thanks, applied and back-patched down to 9.5.  Now for the second
> > problem of this thread..
> >
> Sorry, no clue yet.
> I hacked the perl sources, to hardcoded perl, bison and flex with
> path.It works like this.

Perl has "magic" variable $^X which expands to full path to perl
executable, I wonder why build.pl doesn't  use it to invoke secondary
perl scripts.

--

On 5/6/20 1:14 PM, Victor Wagner wrote:
> В Wed, 6 May 2020 10:21:41 -0300
> Ranier Vilela <ranier.vf@gmail.com> пишет:
>
>> Em qua., 6 de mai. de 2020 às 09:53, Michael Paquier
>> <michael@paquier.xyz> escreveu:
>>
>>> On Tue, May 05, 2020 at 10:16:23AM +0300, Victor Wagner wrote:  
>>>> I agree, it is better.  
>>> Thanks, applied and back-patched down to 9.5.  Now for the second
>>> problem of this thread..
>>>  
>> Sorry, no clue yet.
>> I hacked the perl sources, to hardcoded perl, bison and flex with
>> path.It works like this.
> Perl has "magic" variable $^X which expands to full path to perl
> executable, I wonder why build.pl doesn't  use it to invoke secondary 
> perl scripts.
>

We assume perl, flex and bison are in the PATH. That doesn't seem
unreasonable, it's worked well for quite a long time.


cheers


andrew


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

On Wed, May 06, 2020 at 02:11:34PM -0400, Andrew Dunstan wrote:
> We assume perl, flex and bison are in the PATH. That doesn't seem
> unreasonable, it's worked well for quite a long time.

I recall that it is an assumption we rely on since MSVC scripts are
around, and that's rather easy to configure, so it seems to me that
changing things now would just introduce annoying changes for anybody
(developers, maintainers) using this stuff.
--
Michael

On Wed, May 06, 2020 at 03:58:15PM -0300, Ranier Vilela wrote:
> Hacking pgbison.pl, to print PATH, shows that the path inside pgbison.pl,
> returned to being the original, without the addition of c:\perl\bin;c:\bin.
> my $out = $ENV{PATH};
> print "Path after system call=$out\n";
> Path after system
> call=...C:\Users\ranier\AppData\Local\Microsoft\WindowsApps;;
> The final part lacks: c:\perl\bin;c:\bin
>
> Now I need to find out why the path is being reset, within the perl scripts.

FWIW, we have a buildfarm animal called drongo that runs with VS 2019,
that uses Python, and that is now happy.  One of my own machines uses
VS 2019 as well and I have yet to see what you are describing here.
Perhaps that's related to a difference in the version of perl you are
using and the version of that any others?
--
Michael

В Thu, 7 May 2020 09:14:33 +0900
Michael Paquier <michael@paquier.xyz> пишет:

> On Wed, May 06, 2020 at 03:58:15PM -0300, Ranier Vilela wrote:
> > Hacking pgbison.pl, to print PATH, shows that the path inside
> > pgbison.pl, returned to being the original, without the addition of
> > c:\perl\bin;c:\bin. my $out = $ENV{PATH};
> > print "Path after system call=$out\n";
> > Path after system
> > call=...C:\Users\ranier\AppData\Local\Microsoft\WindowsApps;;
> > The final part lacks: c:\perl\bin;c:\bin
> >
> > Now I need to find out why the path is being reset, within the perl
> > scripts.
>
> FWIW, we have a buildfarm animal called drongo that runs with VS 2019,
> that uses Python, and that is now happy.  One of my own machines uses
> VS 2019 as well and I have yet to see what you are describing here.
> Perhaps that's related to a difference in the version of perl you are
> using and the version of that any others?


I doubt so. I have different machines with perl from 5.22 to 5.30 but
none of tham exibits such weird behavoir.

Perhaps problem is that Ranier calls vcvars64.bat from the menu, and
then calls msbuild such way that is becames unrelated process.

Obvoisly buildfarm animal doesn't use menu and then starts build
process from same CMD.EXE process, that it called vcvarsall.but into.

It is same in all OSes - Windows, *nix and even MS-DOS - there is no
way to change environment of parent process. You can change environment
of current process (and if this process is command interpreter you can
do so by sourcing script into it. In windows this misleadingly called
'CALL', but it executes commands from command file in the current
shell, not in subshell) you can pass enivronment to the child
processes. But you can never affect environment of the parent or
sibling process.

The only exception is - if you know that some process would at startup
read environment vars from some file or registry, you can modify this
source in unrelated process.

So, if you want perl in path of msbuild, started from Visual Studio,
you should either first set path in CMD.EXE, then type command to start
Studio from this very  command window, or set path via control panel
dialog (which modified registry). Later is what I usially do on machines
wher I compile postgres.




--



> --
> Michael

В Wed, 6 May 2020 21:23:57 -0300
Ranier Vilela <ranier.vf@gmail.com> пишет:

>
> The perl is:
> Win32 strawberry-perl 5.30.1.1
>

This perl would have problems when compiling PL/Perl (see my letter
about week ago), but it have no problems running various build scripts
for Postgres. I'm using it with MSVisualStudio 2019 and only unexpected
thing I've encountered is that it comes with its own  patch.exe, which
doesn't like unix-style end-of-lines in patches (but OK with them in
patched files).


> regards,
> Ranier VIlela
--

On Wed, May 06, 2020 at 12:17:03AM +0200, Juan José Santamaría Flecha wrote:
> Please forgive me if I am being too nitpicky, but I find the comments a
> little too verbose, a usage format might be more visual and easier to
> explain:
>
> Usage: build [[CONFIGURATION] COMPONENT]
>
> The options are  case-insensitive.
> CONFIGURATION sets the configuration to build, "debug" or "release" (by
> default).
> COMPONENT defines a component to build. An empty option means all
> components.

Your comment makes sense to me.  What about the attached then?  On top
of documenting the script usage in the code, let's trigger it if it
gets called with more than 3 arguments.  What do you think?

FWIW, I forgot to mention that I don't think those warnings are worth
a backpatch.  No objections with improving things on HEAD of course.
--
Michael

Hi Michael,

I performed a quick test for the path "msvc-build-init-v2.patch" using
below cases:

1. perl build.pl
2. perl build.pl debug psql
3. perl build.pl RELEASE psql
4. perl build.pl deBUG psql
5. perl build.pl psql
The above cases (case-insensitive) are all working great without any
warning for latest master branch.

When build with more than 3 parameters, yes, I got below expected
message as well.

c:\Users\david\Downloads\postgres\src\tools\msvc>perl build.pl DEbug
psql pg_baseback
Usage: build.pl [ [ <configuration> ] <component> ]
Options are case-insensitive.
configuration: Release | Debug. This sets the configuration
to build. Default is Release.
component: name of component to build. An empty value means
to build all components.


However, if I ask for help in a typical Windows' way, i.e, "perl
build.pl -help" or "perl build.pl /?", I got some messages like below,

c:\Users\david\Downloads\postgres\src\tools\msvc>perl build.pl -help
Detected hardware platform: Win32
Files src/bin/pgbench/exprscan.l
Files src/bin/pgbench/exprparse.y
Files src/bin/psql/psqlscanslash.l
Files contrib/cube/cubescan.l
Files contrib/cube/cubeparse.y
Files contrib/seg/segscan.l
Files contrib/seg/segparse.y
Generating configuration headers...
Microsoft (R) Build Engine version 16.5.1+4616136f8 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

MSBUILD : error MSB1001: Unknown switch.
Switch: -help.vcxproj

For switch syntax, type "MSBuild -help"

c:\Users\david\Downloads\postgres\src\tools\msvc>perl build.pl /?
Detected hardware platform: Win32
Files src/bin/pgbench/exprscan.l
Files src/bin/pgbench/exprparse.y
Files src/bin/psql/psqlscanslash.l
Files contrib/cube/cubescan.l
Files contrib/cube/cubeparse.y
Files contrib/seg/segscan.l
Files contrib/seg/segparse.y
Generating configuration headers...
Microsoft (R) Build Engine version 16.5.1+4616136f8 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

MSBUILD : error MSB1001: Unknown switch.
Switch: /?.vcxproj

For switch syntax, type "MSBuild -help"

It would be a bonus if the build.pl can support the "help" in Windows' way.


Thanks,

David

On 2020-05-06 10:45 p.m., Michael Paquier wrote:
> On Wed, May 06, 2020 at 12:17:03AM +0200, Juan José Santamaría Flecha wrote:
>> Please forgive me if I am being too nitpicky, but I find the comments a
>> little too verbose, a usage format might be more visual and easier to
>> explain:
>>
>> Usage: build [[CONFIGURATION] COMPONENT]
>>
>> The options are  case-insensitive.
>> CONFIGURATION sets the configuration to build, "debug" or "release" (by
>> default).
>> COMPONENT defines a component to build. An empty option means all
>> components.
> Your comment makes sense to me.  What about the attached then?  On top
> of documenting the script usage in the code, let's trigger it if it
> gets called with more than 3 arguments.  What do you think?
>
> FWIW, I forgot to mention that I don't think those warnings are worth
> a backpatch.  No objections with improving things on HEAD of course.
> --
> Michael
--
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca

On Sat, Jul 04, 2020 at 09:17:52PM +0200, Juan José Santamaría Flecha wrote:
> Going through the open items in the commitfest, I see that this patch has
> not been pushed. It still applies and solves the warning so, I am marking
> it as RFC.

Thanks, applied.  I was actually waiting to see if you had more
comments.

> Adding a help option is a new feature, that can have its own patch without
> delaying this one any further.

Yep.  And I am not sure if that's worth worrying either.
--
Michael