Thread: Reduce the number of special cases to build contrib modules on windows
Hi, At the moment we do very basic parsing of makefiles to build the visual studio project file in order to build our contrib modules on MSVC. This parsing is quite basic and still requires a number of special cases to get enough information into the project file in order for the build to succeed. It might be nice if we could reduce some of those special cases so that: a) We reduce the amount of work specific to windows when we add new contrib modules, and; b) We can work towards a better way for people to build their own extensions on windows. I admit to not being much of an expert in either perl or make, but I came up with the attached which does allow a good number of the special cases to be removed. I'm keen to get some feedback on this idea. Patch attached. David
Attachment
Hi,
On 2020-11-02 20:34:28 +1300, David Rowley wrote:
> It might be nice if we could reduce some of those special cases so
> that:
>
> a) We reduce the amount of work specific to windows when we add new
> contrib modules, and;
> b) We can work towards a better way for people to build their own
> extensions on windows.
A worthy goal.
> diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
> index 90594bd41b..491a465e2f 100644
> --- a/src/tools/msvc/Mkvcbuild.pm
> +++ b/src/tools/msvc/Mkvcbuild.pm
> @@ -32,16 +32,13 @@ my $libpq;
> my @unlink_on_exit;
>
> # Set of variables for modules in contrib/ and src/test/modules/
> -my $contrib_defines = { 'refint' => 'REFINT_VERBOSE' };
> -my @contrib_uselibpq = ('dblink', 'oid2name', 'postgres_fdw', 'vacuumlo');
> -my @contrib_uselibpgport = ('oid2name', 'pg_standby', 'vacuumlo');
> -my @contrib_uselibpgcommon = ('oid2name', 'pg_standby', 'vacuumlo');
> +my $contrib_defines = undef;
> +my @contrib_uselibpq = undef;
> +my @contrib_uselibpgport = ('pg_standby');
> +my @contrib_uselibpgcommon = ('pg_standby');
> my $contrib_extralibs = undef;
> my $contrib_extraincludes = { 'dblink' => ['src/backend'] };
> -my $contrib_extrasource = {
> - 'cube' => [ 'contrib/cube/cubescan.l', 'contrib/cube/cubeparse.y' ],
> - 'seg' => [ 'contrib/seg/segscan.l', 'contrib/seg/segparse.y' ],
> -};
> +my $contrib_extrasource = undef;
Hm - Is that all the special case stuff we get rid of?
What's with the now unef'd arrays/hashes? First, wouldn't an empty array be
more appropriate? Second, can we just get rid of them?
And why is the special stuff for pg_standby still needed?
> my @contrib_excludes = (
> 'bool_plperl', 'commit_ts',
> 'hstore_plperl', 'hstore_plpython',
> @@ -163,7 +160,7 @@ sub mkvcbuild
> $postgres = $solution->AddProject('postgres', 'exe', '', 'src/backend');
> $postgres->AddIncludeDir('src/backend');
> $postgres->AddDir('src/backend/port/win32');
> - $postgres->AddFile('src/backend/utils/fmgrtab.c');
> + $postgres->AddFile('src/backend/utils/fmgrtab.c', 1);
> $postgres->ReplaceFile('src/backend/port/pg_sema.c',
> 'src/backend/port/win32_sema.c');
> $postgres->ReplaceFile('src/backend/port/pg_shmem.c',
> @@ -316,8 +313,8 @@ sub mkvcbuild
Why do so many places need this new parameter? Looks like all explicit
calls use it? Can't we just use it by default, using a separate function
for the internal cases? Would make this a lot more readable...
> my $isolation_tester =
> $solution->AddProject('isolationtester', 'exe', 'misc');
> - $isolation_tester->AddFile('src/test/isolation/isolationtester.c');
> - $isolation_tester->AddFile('src/test/isolation/specparse.y');
> - $isolation_tester->AddFile('src/test/isolation/specscanner.l');
> - $isolation_tester->AddFile('src/test/isolation/specparse.c');
> + $isolation_tester->AddFile('src/test/isolation/isolationtester.c', 1);
> + $isolation_tester->AddFile('src/test/isolation/specparse.y', 1);
> + $isolation_tester->AddFile('src/test/isolation/specscanner.l', 1);
> + $isolation_tester->AddFile('src/test/isolation/specparse.c', 1);
> $isolation_tester->AddIncludeDir('src/test/isolation');
> $isolation_tester->AddIncludeDir('src/port');
> $isolation_tester->AddIncludeDir('src/test/regress');
> @@ -342,8 +339,8 @@ sub mkvcbuild
Why aren't these dealth with using the .c->.l/.y logic you added?
> + # Process custom compiler flags
> + if ($mf =~ /^PG_CPPFLAGS\s*=\s*(.*)$/mg)
Probably worth mentioning in pgxs.mk or such.
> + {
> + foreach my $flag (split /\s+/, $1)
> + {
> + if ($flag =~ /^-D(.*)$/)
> + {
> + foreach my $proj (@projects)
> + {
> + $proj->AddDefine($1);
> + }
> + }
> + elsif ($flag =~ /^-I(.*)$/)
> + {
> + foreach my $proj (@projects)
> + {
> + if ($1 eq '$(libpq_srcdir)')
> + {
> + $proj->AddIncludeDir('src\interfaces\libpq');
> + $proj->AddReference($libpq);
> + }
Why just libpq?
> +# Handle makefile rules for when file to be added to the project
> +# does not exist. Returns 1 when the original file add should be
> +# skipped.
> +sub AdditionalFileRules
> +{
> + my $self = shift;
> + my $fname = shift;
> + my ($ext) = $fname =~ /(\.[^.]+)$/;
> +
> + # For missing .c files, check if a .l file of the same name
> + # exists and add that too.
> + if ($ext eq ".c")
> + {
> + my $filenoext = $fname;
> + $filenoext =~ s{\.[^.]+$}{};
> + if (-e "$filenoext.l")
> + {
> + AddFile($self, "$filenoext.l", 0);
> + return 1;
> + }
> + if (-e "$filenoext.y")
> + {
> + AddFile($self, "$filenoext.y", 0);
> + return 0;
> + }
> + }
Aren't there related rules for .h?
Greetings,
Andres Freund
Thank you for looking at this.
On Tue, 3 Nov 2020 at 09:49, Andres Freund <andres@anarazel.de> wrote:
> > diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
> > index 90594bd41b..491a465e2f 100644
> > --- a/src/tools/msvc/Mkvcbuild.pm
> > +++ b/src/tools/msvc/Mkvcbuild.pm
> > @@ -32,16 +32,13 @@ my $libpq;
> > my @unlink_on_exit;
> >
> > # Set of variables for modules in contrib/ and src/test/modules/
> > -my $contrib_defines = { 'refint' => 'REFINT_VERBOSE' };
> > -my @contrib_uselibpq = ('dblink', 'oid2name', 'postgres_fdw', 'vacuumlo');
> > -my @contrib_uselibpgport = ('oid2name', 'pg_standby', 'vacuumlo');
> > -my @contrib_uselibpgcommon = ('oid2name', 'pg_standby', 'vacuumlo');
> > +my $contrib_defines = undef;
> > +my @contrib_uselibpq = undef;
> > +my @contrib_uselibpgport = ('pg_standby');
> > +my @contrib_uselibpgcommon = ('pg_standby');
> > my $contrib_extralibs = undef;
> > my $contrib_extraincludes = { 'dblink' => ['src/backend'] };
> > -my $contrib_extrasource = {
> > - 'cube' => [ 'contrib/cube/cubescan.l', 'contrib/cube/cubeparse.y' ],
> > - 'seg' => [ 'contrib/seg/segscan.l', 'contrib/seg/segparse.y' ],
> > -};
> > +my $contrib_extrasource = undef;
>
> Hm - Is that all the special case stuff we get rid of?
What else did you have in mind?
> What's with the now unef'd arrays/hashes? First, wouldn't an empty array be
> more appropriate? Second, can we just get rid of them?
Yes, those should be empty hashtables/arrays. I've changed that.
We could get rid of the variables too. I've just left them in there
for now as I wasn't sure if it might be a good idea to keep them for
if we really need to brute force something in the future. I found
parsing makefiles quite tedious, so it didn't seem unrealistic to me
that someone might struggle in the future to make something work.
> And why is the special stuff for pg_standby still needed?
I'm not much of an expert, but I didn't see anything in the makefile
for pg_standby that indicates we should link libpgport or libpgcommon.
It would be good if someone could explain how that works.
> > my @contrib_excludes = (
> > 'bool_plperl', 'commit_ts',
> > 'hstore_plperl', 'hstore_plpython',
> > @@ -163,7 +160,7 @@ sub mkvcbuild
> > $postgres = $solution->AddProject('postgres', 'exe', '', 'src/backend');
> > $postgres->AddIncludeDir('src/backend');
> > $postgres->AddDir('src/backend/port/win32');
> > - $postgres->AddFile('src/backend/utils/fmgrtab.c');
> > + $postgres->AddFile('src/backend/utils/fmgrtab.c', 1);
> > $postgres->ReplaceFile('src/backend/port/pg_sema.c',
> > 'src/backend/port/win32_sema.c');
> > $postgres->ReplaceFile('src/backend/port/pg_shmem.c',
> > @@ -316,8 +313,8 @@ sub mkvcbuild
>
> Why do so many places need this new parameter? Looks like all explicit
> calls use it? Can't we just use it by default, using a separate function
> for the internal cases? Would make this a lot more readable...
That makes sense. I've updated the patch to have AddFile() add any
additional files always then I've also added a new function named
AddFileConditional which does what AddFile(..., 0) did.
> > my $isolation_tester =
> > $solution->AddProject('isolationtester', 'exe', 'misc');
> > - $isolation_tester->AddFile('src/test/isolation/isolationtester.c');
> > - $isolation_tester->AddFile('src/test/isolation/specparse.y');
> > - $isolation_tester->AddFile('src/test/isolation/specscanner.l');
> > - $isolation_tester->AddFile('src/test/isolation/specparse.c');
> > + $isolation_tester->AddFile('src/test/isolation/isolationtester.c', 1);
> > + $isolation_tester->AddFile('src/test/isolation/specparse.y', 1);
> > + $isolation_tester->AddFile('src/test/isolation/specscanner.l', 1);
> > + $isolation_tester->AddFile('src/test/isolation/specparse.c', 1);
> > $isolation_tester->AddIncludeDir('src/test/isolation');
> > $isolation_tester->AddIncludeDir('src/port');
> > $isolation_tester->AddIncludeDir('src/test/regress');
> > @@ -342,8 +339,8 @@ sub mkvcbuild
>
> Why aren't these dealth with using the .c->.l/.y logic you added?
Yeah, some of those could be removed. I mostly only paid attention to
contrib though.
> > + # Process custom compiler flags
> > + if ($mf =~ /^PG_CPPFLAGS\s*=\s*(.*)$/mg)
>
> Probably worth mentioning in pgxs.mk or such.
I'm not quite sure I understand what you mean here.
> > + {
> > + foreach my $flag (split /\s+/, $1)
> > + {
> > + if ($flag =~ /^-D(.*)$/)
> > + {
> > + foreach my $proj (@projects)
> > + {
> > + $proj->AddDefine($1);
> > + }
> > + }
> > + elsif ($flag =~ /^-I(.*)$/)
> > + {
> > + foreach my $proj (@projects)
> > + {
> > + if ($1 eq '$(libpq_srcdir)')
> > + {
> > + $proj->AddIncludeDir('src\interfaces\libpq');
> > + $proj->AddReference($libpq);
> > + }
>
> Why just libpq?
I've only gone as far as making the existing contrib modules build.
Likely there's more to be done there.
> > +# Handle makefile rules for when file to be added to the project
> > +# does not exist. Returns 1 when the original file add should be
> > +# skipped.
> > +sub AdditionalFileRules
> > +{
> > + my $self = shift;
> > + my $fname = shift;
> > + my ($ext) = $fname =~ /(\.[^.]+)$/;
> > +
> > + # For missing .c files, check if a .l file of the same name
> > + # exists and add that too.
> > + if ($ext eq ".c")
> > + {
> > + my $filenoext = $fname;
> > + $filenoext =~ s{\.[^.]+$}{};
> > + if (-e "$filenoext.l")
> > + {
> > + AddFile($self, "$filenoext.l", 0);
> > + return 1;
> > + }
> > + if (-e "$filenoext.y")
> > + {
> > + AddFile($self, "$filenoext.y", 0);
> > + return 0;
> > + }
> > + }
>
> Aren't there related rules for .h?
I've only gone as far as making the existing contrib modules build.
Likely there's more to be done there.
David
Attachment
Re: Reduce the number of special cases to build contrib modules on windows
From
Alvaro Herrera
Date:
On 2020-Nov-06, David Rowley wrote:
> +# Handle makefile rules for when file to be added to the project
> +# does not exist. Returns 1 when the original file add should be
> +# skipped.
> +sub FindAndAddAdditionalFiles
> +{
> + my $self = shift;
> + my $fname = shift;
> + my ($ext) = $fname =~ /(\.[^.]+)$/;
> +
> + # For .c files, check if a .l file of the same name exists and add that
> + # too.
> + if ($ext eq ".c")
> + {
> + my $filenoext = $fname;
> + $filenoext =~ s{\.[^.]+$}{};
I think you can make this simpler by capturing both the basename and the
extension in one go. For example,
$fname =~ /(.*)(\.[^.]+)$/;
$filenoext = $1;
$ext = $2;
so you avoid the second =~ statement.
> + if (-e "$filenoext.l")
> + {
> + AddFileConditional($self, "$filenoext.l");
> + return 1;
> + }
> + if (-e "$filenoext.y")
> + {
> + AddFileConditional($self, "$filenoext.y");
Maybe DRY like
for my $ext (".l", ".y") {
my $file = $filenoext . $ext;
AddFileConditional($self, $file) if -f $file;
return 1;
}
Note: comment says "check if a .l file" and then checks both .l and .y.
Probably want to update the comment ...
On Tue, 10 Nov 2020 at 03:07, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2020-Nov-06, David Rowley wrote:
>
> > +# Handle makefile rules for when file to be added to the project
> > +# does not exist. Returns 1 when the original file add should be
> > +# skipped.
> > +sub FindAndAddAdditionalFiles
> > +{
> > + my $self = shift;
> > + my $fname = shift;
> > + my ($ext) = $fname =~ /(\.[^.]+)$/;
> > +
> > + # For .c files, check if a .l file of the same name exists and add that
> > + # too.
> > + if ($ext eq ".c")
> > + {
> > + my $filenoext = $fname;
> > + $filenoext =~ s{\.[^.]+$}{};
>
> I think you can make this simpler by capturing both the basename and the
> extension in one go. For example,
>
> $fname =~ /(.*)(\.[^.]+)$/;
> $filenoext = $1;
> $ext = $2;
>
> so you avoid the second =~ statement.
Thanks. That's neater.
> > + if (-e "$filenoext.l")
> > + {
> > + AddFileConditional($self, "$filenoext.l");
> > + return 1;
> > + }
> > + if (-e "$filenoext.y")
> > + {
> > + AddFileConditional($self, "$filenoext.y");
>
> Maybe DRY like
>
> for my $ext (".l", ".y") {
> my $file = $filenoext . $ext;
> AddFileConditional($self, $file) if -f $file;
> return 1;
> }
I did adapt that part of the code, but not exactly to what's above.
The return there would cause us to return from the function after the
first iteration.
> Note: comment says "check if a .l file" and then checks both .l and .y.
> Probably want to update the comment ...
Updated.
I've attached the v3 patch.
I'm still working through some small differences in some of the
.vcxproj files. I've been comparing these by copying *.vcxproj out to
another directory with patched and unpatched then diffing the
directory. See attached txt file with those diffs. Here's a summary of
some of them:
1. There are a few places that libpq gets linked where it previously did not.
2. REFINT_VERBOSE gets defined in a few more places than it did
previously. This makes it closer to what happens on Linux anyway, if
you look at the Make output from contrib/spi/Makefile you'll see
-DREFINT_VERBOSE in there for autoinc
3. LOWER_NODE gets defined in ltree now where it wasn't before. It's
defined on Linux. Unsure why it wasn't before on Windows.
David
Attachment
Re: Reduce the number of special cases to build contrib modules on windows
From
Michael Paquier
Date:
On Wed, Nov 11, 2020 at 11:01:57AM +1300, David Rowley wrote: > I'm still working through some small differences in some of the > .vcxproj files. I've been comparing these by copying *.vcxproj out to > another directory with patched and unpatched then diffing the > directory. See attached txt file with those diffs. Here's a summary of > some of them: Thanks. It would be good to not have those diffs if not necessary. > 1. There are a few places that libpq gets linked where it previously did not. It seems to me that your patch is doing the right thing for adminpack and that its Makefile has no need to include a reference to libpq source path, no? For dblink and postgres_fdw, the duplication comes from PG_CPPFLAGS. It does not matter much in practice, but it would be nice to not have unnecessary data in the project files. One thing that could be done is to make Project.pm more aware of the uniqueness of the elements included. But, do we really need -I$(libpq_srcdir) there anyway? From what I can see, we have all the paths in -I we'd actually need with or without USE_PGXS. > 2. REFINT_VERBOSE gets defined in a few more places than it did > previously. This makes it closer to what happens on Linux anyway, if > you look at the Make output from contrib/spi/Makefile you'll see > -DREFINT_VERBOSE in there for autoinc. Indeed. > 3. LOWER_NODE gets defined in ltree now where it wasn't before. It's > defined on Linux. Unsure why it wasn't before on Windows. Your patch is grabbing the value of PG_CPPFLAGS from ltree's Makefile, which is fine. We may be able to remove this flag and rely on pg_tolower() instead in the long run? I am not sure about FLG_CANLOOKSIGN() though. -- Michael
Attachment
On Wed, 11 Nov 2020 at 13:44, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Nov 11, 2020 at 11:01:57AM +1300, David Rowley wrote: > > I'm still working through some small differences in some of the > > .vcxproj files. I've been comparing these by copying *.vcxproj out to > > another directory with patched and unpatched then diffing the > > directory. See attached txt file with those diffs. Here's a summary of > > some of them: > > Thanks. It would be good to not have those diffs if not necessary. > > > 1. There are a few places that libpq gets linked where it previously did not. > > It seems to me that your patch is doing the right thing for adminpack > and that its Makefile has no need to include a reference to libpq > source path, no? Yeah. Likely a separate commit should remove the -I$(libpq_srcdir) from adminpack and old_snapshot > For dblink and postgres_fdw, the duplication comes from PG_CPPFLAGS. > It does not matter much in practice, but it would be nice to not have > unnecessary data in the project files. One thing that could be done > is to make Project.pm more aware of the uniqueness of the elements > included. But, do we really need -I$(libpq_srcdir) there anyway? > From what I can see, we have all the paths in -I we'd actually need > with or without USE_PGXS. I've changed the patch to do that for the includes. I'm now putting the list of include directories in a hash table to get rid of the duplicates. This does shuffle the order of them around a bit. I've done the same for references too. > > 3. LOWER_NODE gets defined in ltree now where it wasn't before. It's > > defined on Linux. Unsure why it wasn't before on Windows. > > Your patch is grabbing the value of PG_CPPFLAGS from ltree's > Makefile, which is fine. We may be able to remove this flag and rely > on pg_tolower() instead in the long run? I am not sure about > FLG_CANLOOKSIGN() though. I didn't look in detail, but it looks like if we define LOWER_NODE on Windows that it might break pg_upgrade. I guess you could say it's partially broken now as the behaviour there will depend on if you build using Visual Studio or cygwin. We'd define LOWER_NODE on cygwin but not on VS. Looks like a pg_upgrade might be problematic there today. It feels a bit annoying to add some special case to the script to maintain the status quo there. An alternative to that would be to modify the .c code at #ifdef LOWER_NODE to also check we're not building on VS. Neither option seems nice. I've attached the updated patch and also a diff showing the changes in the *.vcxproj files. There are quite a few places where the hash table code for includes and references gets rid of duplicates that already exist today. For example pgbench.vcxproj references libpgport.vcxproj and libpgcommon.vcxproj twice. David
Attachment
Re: Reduce the number of special cases to build contrib modules on windows
From
Michael Paquier
Date:
On Tue, Dec 22, 2020 at 11:24:40PM +1300, David Rowley wrote: > On Wed, 11 Nov 2020 at 13:44, Michael Paquier <michael@paquier.xyz> wrote: >> It seems to me that your patch is doing the right thing for adminpack >> and that its Makefile has no need to include a reference to libpq >> source path, no? > > Yeah. Likely a separate commit should remove the -I$(libpq_srcdir) > from adminpack and old_snapshot I have begun a new thread about this point as that's a separate topic. I did not see other places in need of a similar cleanup: https://www.postgresql.org/message-id/X+LQpfLyk7jgzUki@paquier.xyz > I didn't look in detail, but it looks like if we define LOWER_NODE on > Windows that it might break pg_upgrade. I guess you could say it's > partially broken now as the behaviour there will depend on if you > build using Visual Studio or cygwin. We'd define LOWER_NODE on cygwin > but not on VS. Looks like a pg_upgrade might be problematic there > today. > > It feels a bit annoying to add some special case to the script to > maintain the status quo there. An alternative to that would be to > modify the .c code at #ifdef LOWER_NODE to also check we're not > building on VS. Neither option seems nice. Hmm. It seems that you are right here. This influences lquery parsing so it may be nasty and this exists since ltree is present in the tree (2002). I think that I would choose the update in the C code and remove LOWER_NODE while keeping the scripts clean, and documenting directly in the code why this compatibility issue exists. REFINT_VERBOSE is no problem, fortunately. > I've attached the updated patch and also a diff showing the changes in > the *.vcxproj files. Thanks! > There are quite a few places where the hash table code for includes > and references gets rid of duplicates that already exist today. For > example pgbench.vcxproj references libpgport.vcxproj and > libpgcommon.vcxproj twice. The diffs look clean. dblink has lost src/backend/, there are the additions of REFINT_VERBOSE and LOWER_NODE but the bulk of the diffs comes from a change in the order of items listed, while removing duplicates. I have tested your patch, and this is causing compilation failures for hstore_plpython, jsonb_plpython and ltree_plpython. So AddTransformModule is missing something here when compiling with Python. -- Michael
Attachment
On Wed, 23 Dec 2020 at 18:46, Michael Paquier <michael@paquier.xyz> wrote: > I have begun a new thread about this point as that's a separate > topic. I did not see other places in need of a similar cleanup: > https://www.postgresql.org/message-id/X+LQpfLyk7jgzUki@paquier.xyz Thanks. I'll look at that shortly. > > I didn't look in detail, but it looks like if we define LOWER_NODE on > > Windows that it might break pg_upgrade. I guess you could say it's > > partially broken now as the behaviour there will depend on if you > > build using Visual Studio or cygwin. We'd define LOWER_NODE on cygwin > > but not on VS. Looks like a pg_upgrade might be problematic there > > today. > > > > It feels a bit annoying to add some special case to the script to > > maintain the status quo there. An alternative to that would be to > > modify the .c code at #ifdef LOWER_NODE to also check we're not > > building on VS. Neither option seems nice. > > Hmm. It seems that you are right here. This influences lquery > parsing so it may be nasty and this exists since ltree is present in > the tree (2002). I think that I would choose the update in the C code > and remove LOWER_NODE while keeping the scripts clean, and documenting > directly in the code why this compatibility issue exists. > REFINT_VERBOSE is no problem, fortunately. I ended up modifying each place in the C code where we check LOWER_NODE. I found 2 places, one in crc32.c and another in ltree.h. I added the same comment to both to explain why there's a check for !defined(_MSC_VER) there. I'm not particularly happy about this code, but I don't really see what else to do right now. > I have tested your patch, and this is causing compilation failures for > hstore_plpython, jsonb_plpython and ltree_plpython. So > AddTransformModule is missing something here when compiling with > Python. Oh thanks for finding that. That was due to some incorrect Perl code I'd written to add the includes from one project into another. Fixed by: - $p->AddIncludeDir(join(";", $pl_proj->{includes})); + foreach my $inc (keys %{ $pl_proj->{includes} } ) + { + $p->AddIncludeDir($inc); + } + David
Attachment
On Wed, 30 Dec 2020 at 10:03, David Rowley <dgrowleyml@gmail.com> wrote: > > On Wed, 23 Dec 2020 at 18:46, Michael Paquier <michael@paquier.xyz> wrote: > > I have tested your patch, and this is causing compilation failures for > > hstore_plpython, jsonb_plpython and ltree_plpython. So > > AddTransformModule is missing something here when compiling with > > Python. > > Oh thanks for finding that. That was due to some incorrect Perl code > I'd written to add the includes from one project into another. Fixed > by: I accidentally attached the wrong patch before. Now attaching the correct one. David
Attachment
Re: Reduce the number of special cases to build contrib modules on windows
From
Michael Paquier
Date:
On Wed, Dec 30, 2020 at 10:07:29AM +1300, David Rowley wrote:
> -#ifdef LOWER_NODE
> +/*
> + * Below we ignore the fact that LOWER_NODE is defined when compiling with
> + * MSVC. The reason for this is that earlier versions of the MSVC build
> + * scripts failed to define LOWER_NODE. More recent version of the MSVC
> + * build scripts parse makefiles which results in LOWER_NODE now being
> + * defined. We check for _MSC_VER here so as not to break pg_upgrade when
> + * upgrading from versions MSVC versions where LOWER_NODE was not defined.
> + */
> +#if defined(LOWER_NODE) && !defined(_MSC_VER)
> #include <ctype.h>
> #define TOLOWER(x) tolower((unsigned char) (x))
> #else
While on it, do you think that it would be more readable if we remove
completely LOWER_NODE and use only a check based on _MSC_VER for those
two files in ltree? This could also be handled as a separate change.
> + foreach my $line (split /\n/, $mf)
> + {
> + if ($line =~ /^[A-Za-z0-9_]*\.o:\s(.*)/)
> + {
> + foreach my $file (split /\s+/, $1)
> + {
> + foreach my $proj (@projects)
> + {
> + $proj->AddFileConditional("$subdir/$n/$file");
> + }
> + }
> + }
> + }
Looking closer at this change, I don't think that this is completely
correct and that could become a trap. This is adding quite a bit of
complexity to take care of contrib_extrasource getting empty, and it
actually overlaps with the handling of OBJS done in AddDir(), no?
--
Michael
Attachment
On Wed, 3 Mar 2021 at 22:37, David Rowley <dgrowleyml@gmail.com> wrote: > I've attached a rebased patch. I've rebased this again. I also moved away from using hash tables for storing references and libraries. I was having some problems getting psql to compile due to the order of the dependencies being reversed due to the order being at the mercy of Perl's hash function. There's mention of this in Makefile.global.in: # libpq_pgport is for use by client executables (not libraries) that use libpq. # We force clients to pull symbols from the non-shared libraries libpgport # and libpgcommon rather than pulling some libpgport symbols from libpq just # because libpq uses those functions too. This makes applications less # dependent on changes in libpq's usage of pgport (on platforms where we # don't have symbol export control for libpq). To do this we link to # pgport before libpq. This does cause duplicate -lpgport's to appear # on client link lines, since that also appears in $(LIBS). # libpq_pgport_shlib is the same idea, but for use in client shared libraries. I switched these back to arrays but added an additional check to only add new items to the array if we don't already have an element with the same value. I've attached the diffs in the *.vcxproj files between patched and unpatched. David
Attachment
Re: Reduce the number of special cases to build contrib modules on windows
From
Alvaro Herrera
Date:
> diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
> index ebb169e201..68606a296d 100644
> --- a/src/tools/msvc/MSBuildProject.pm
> +++ b/src/tools/msvc/MSBuildProject.pm
> @@ -310,11 +310,12 @@ sub WriteItemDefinitionGroup
> my $targetmachine =
> $self->{platform} eq 'Win32' ? 'MachineX86' : 'MachineX64';
>
> - my $includes = $self->{includes};
> - unless ($includes eq '' or $includes =~ /;$/)
> + my $includes = "";
> + foreach my $inc (@{ $self->{includes} })
> {
> - $includes .= ';';
> + $includes .= $inc . ";";
> }
Perl note: you can do this more easily as
my $includes = join ';', @{$self->{includes}};
$includes .= ';' unless $includes eq '';
--
Álvaro Herrera 39°49'30"S 73°17'W
Are you not unsure you want to delete Firefox?
[Not unsure] [Not not unsure] [Cancel]
http://smylers.hates-software.com/2008/01/03/566e45b2.html
Re: Reduce the number of special cases to build contrib modules on windows
From
Andrew Dunstan
Date:
On 4/19/21 12:24 PM, Alvaro Herrera wrote:
>> diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
>> index ebb169e201..68606a296d 100644
>> --- a/src/tools/msvc/MSBuildProject.pm
>> +++ b/src/tools/msvc/MSBuildProject.pm
>> @@ -310,11 +310,12 @@ sub WriteItemDefinitionGroup
>> my $targetmachine =
>> $self->{platform} eq 'Win32' ? 'MachineX86' : 'MachineX64';
>>
>> - my $includes = $self->{includes};
>> - unless ($includes eq '' or $includes =~ /;$/)
>> + my $includes = "";
>> + foreach my $inc (@{ $self->{includes} })
>> {
>> - $includes .= ';';
>> + $includes .= $inc . ";";
>> }
> Perl note: you can do this more easily as
>
> my $includes = join ';', @{$self->{includes}};
> $includes .= ';' unless $includes eq '';
>
or even more simply:
my $includes = join ';', @{$self->{includes}}, "";
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Tue, 20 Apr 2021 at 09:28, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 4/19/21 12:24 PM, Alvaro Herrera wrote:
> >> diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
> >> index ebb169e201..68606a296d 100644
> >> --- a/src/tools/msvc/MSBuildProject.pm
> >> +++ b/src/tools/msvc/MSBuildProject.pm
> >> @@ -310,11 +310,12 @@ sub WriteItemDefinitionGroup
> >> my $targetmachine =
> >> $self->{platform} eq 'Win32' ? 'MachineX86' : 'MachineX64';
> >>
> >> - my $includes = $self->{includes};
> >> - unless ($includes eq '' or $includes =~ /;$/)
> >> + my $includes = "";
> >> + foreach my $inc (@{ $self->{includes} })
> >> {
> >> - $includes .= ';';
> >> + $includes .= $inc . ";";
> >> }
> > Perl note: you can do this more easily as
> >
> > my $includes = join ';', @{$self->{includes}};
> > $includes .= ';' unless $includes eq '';
> >
>
> or even more simply:
>
>
> my $includes = join ';', @{$self->{includes}}, "";
Both look more compact. Thanks. I'll include this for the next version.
David
On Mon, Apr 19, 2021 at 5:18 PM David Rowley <dgrowleyml@gmail.com> wrote: > > On Wed, 3 Mar 2021 at 22:37, David Rowley <dgrowleyml@gmail.com> wrote: > > I've attached a rebased patch. > > I've rebased this again. > > I also moved away from using hash tables for storing references and > libraries. I was having some problems getting psql to compile due to > the order of the dependencies being reversed due to the order being at > the mercy of Perl's hash function. There's mention of this in > Makefile.global.in: > > # libpq_pgport is for use by client executables (not libraries) that use libpq. > # We force clients to pull symbols from the non-shared libraries libpgport > # and libpgcommon rather than pulling some libpgport symbols from libpq just > # because libpq uses those functions too. This makes applications less > # dependent on changes in libpq's usage of pgport (on platforms where we > # don't have symbol export control for libpq). To do this we link to > # pgport before libpq. This does cause duplicate -lpgport's to appear > # on client link lines, since that also appears in $(LIBS). > # libpq_pgport_shlib is the same idea, but for use in client shared libraries. > > I switched these back to arrays but added an additional check to only > add new items to the array if we don't already have an element with > the same value. > > I've attached the diffs in the *.vcxproj files between patched and unpatched. The patch does not apply on Head anymore, could you rebase and post a patch. I'm changing the status to "Waiting for Author". Regards, Vignesh
On Thu, 15 Jul 2021 at 04:01, vignesh C <vignesh21@gmail.com> wrote:
> The patch does not apply on Head anymore, could you rebase and post a
> patch. I'm changing the status to "Waiting for Author".
I've rebased this patch and broken it down into 6 individual patches.
0001: Removes an include directory for dblink. This appears like it's
not needed. It was added in ee3b4188a (Jan 2010), but an earlier
commit, 320c7eb8c (June 2008) seems to have made it pointless. It's
still a mystery to me why ee3b4188a would have been required in the
first place.
0002: Parses -D in the CPPFLAGS of Makefiles and uses those in the
MSVC script. It also adjusts the ltree contrib module so that we do
the same LOWER_NODE behaviour as we did before. The MSVC scripts
appear to have mistakenly forgotten to define LOWER_NODE as it is in
the Makefiles.
0003: Is a tidy up patch to make the 'includes' field an array rather
than a string
0004: Adds code to check for duplicate references and libraries before
adding new ones of the same name to the project.
0005: Is mostly a tidy up so that we use AddFile consistently instead
of sometimes doing $self->{files}->{<name>} = 1;
0006: I'm not so sure about. It attempts to do a bit more Makefile
parsing to get rid of contrib_extrasource and the majority of
contrib_uselibpgport and contrib_uselibpgcommon usages.
David
Attachment
- v9-0001-Remove-unneeded-include-directory-in-MSVC-scripts.patch
- v9-0002-Adjust-MSVC-build-scripts-to-parse-Makefiles-for-.patch
- v9-0003-Make-the-includes-field-an-array-in-MSVC-build-sc.patch
- v9-0004-Don-t-duplicate-references-and-libraries-in-MSVC-.patch
- v9-0005-Use-AddFile-consistently-in-MSVC-scripts.patch
- v9-0006-Remove-some-special-cases-from-MSVC-build-scripts.patch
Re: Reduce the number of special cases to build contrib modules on windows
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 15 Jul 2021 at 04:01, vignesh C <vignesh21@gmail.com> wrote:
>> The patch does not apply on Head anymore, could you rebase and post a
>> patch. I'm changing the status to "Waiting for Author".
>
> I've rebased this patch and broken it down into 6 individual patches.
I don't know anything about the MSVC build process, but I figured I
could do a general Perl code review.
> --- a/src/tools/msvc/Mkvcbuild.pm
> +++ b/src/tools/msvc/Mkvcbuild.pm
[…]
> + # Process custom compiler flags
> + if ($mf =~ /^PG_CPPFLAGS\s*=\s*(.*)$/mg || $mf =~ /^override\s*CPPFLAGS\s*(?:[\+\:])?=\s*(.*)$/mg)
^^^^^^^^^^^
This is a very convoluted way of writing [+:]?
> --- a/src/tools/msvc/Project.pm
> +++ b/src/tools/msvc/Project.pm
> @@ -58,7 +58,7 @@ sub AddFiles
>
> while (my $f = shift)
> {
> - $self->{files}->{ $dir . "/" . $f } = 1;
> + AddFile($self, $dir . "/" . $f, 1);
AddFile is a method, so should be called as $self->AddFile(…).
> --- a/src/tools/msvc/Mkvcbuild.pm
> +++ b/src/tools/msvc/Mkvcbuild.pm
> @@ -36,16 +36,12 @@ my @unlink_on_exit;
[…]
> + elsif ($flag =~ /^-I(.*)$/)
> + {
> + foreach my $proj (@projects)
> + {
> + if ($1 eq '$(libpq_srcdir)')
> + {
> + $proj->AddIncludeDir('src\interfaces\libpq');
> + $proj->AddReference($libpq);
> + }
> + }
> + }
It would be better to do the if check outside the for loop.
> --- a/src/tools/msvc/Project.pm
> +++ b/src/tools/msvc/Project.pm
> @@ -51,6 +51,16 @@ sub AddFile
> return;
> }
>
> +sub AddFileAndAdditionalFiles
> +{
> + my ($self, $filename) = @_;
> + if (FindAndAddAdditionalFiles($self, $filename) != 1)
Again, FindAndAddAdditionalFiles is a method and should be called as
$self->FindAndAddAdditionalFiles($filename).
> + {
> + $self->{files}->{$filename} = 1;
> + }
> + return;
> +}
- ilmari
David Rowley <dgrowleyml@gmail.com> writes:
> 0001: Removes an include directory for dblink. This appears like it's
> not needed. It was added in ee3b4188a (Jan 2010), but an earlier
> commit, 320c7eb8c (June 2008) seems to have made it pointless. It's
> still a mystery to me why ee3b4188a would have been required in the
> first place.
FWIW, I poked around in the mailing list archives around that date
and couldn't find any supporting discussion. It does seem like it
shouldn't be needed, given that dblink's Makefile does no such thing.
I'd suggest just pushing your 0001 and seeing if the buildfarm
complains.
> 0002: Parses -D in the CPPFLAGS of Makefiles and uses those in the
> MSVC script. It also adjusts the ltree contrib module so that we do
> the same LOWER_NODE behaviour as we did before. The MSVC scripts
> appear to have mistakenly forgotten to define LOWER_NODE as it is in
> the Makefiles.
The LOWER_NODE situation seems like a mess, but I think the right fix
is to remove -DLOWER_NODE from the Makefile altogether and move the
responsibility into the C code. You could have ltree.h do
#if !defined(_MSC_VER)
#define LOWER_NODE 1
#endif
and put the explanatory comment on that, not on the uses of the flag.
Haven't looked at the rest.
regards, tom lane
Re: Reduce the number of special cases to build contrib modules on windows
From
Alvaro Herrera
Date:
On 2021-Jul-28, David Rowley wrote:
> 0003: Is a tidy up patch to make the 'includes' field an array rather
> than a string
In this one, you can avoid turning one line into four with map,
- $p->AddIncludeDir($pl_proj->{includes});
+ foreach my $inc (@{ $pl_proj->{includes} })
+ {
+ $p->AddIncludeDir($inc);
+ }
Instead of that you can do something like this:
+ map { $p->AddIncludeDir($_); } @{$pl_proj->{includes}};
> 0004: Adds code to check for duplicate references and libraries before
> adding new ones of the same name to the project.
I think using the return value of grep as a boolean is confusing. It
seems more legible to compare to 0. So instead of this:
+ if (! grep { $_ eq $ref} @{ $self->{references} })
+ {
+ push @{ $self->{references} }, $ref;
+ }
use something like:
+ if (grep { $_ eq $ref} @{ $self->{references} } == 0)
> 0006: I'm not so sure about. It attempts to do a bit more Makefile
> parsing to get rid of contrib_extrasource and the majority of
> contrib_uselibpgport and contrib_uselibpgcommon usages.
I wonder if we could fix up libpq_pipeline's Makefile somehow to get rid
of the remaining ones.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Reduce the number of special cases to build contrib modules on windows
From
Andrew Dunstan
Date:
On 7/27/21 11:01 AM, Alvaro Herrera wrote:
> On 2021-Jul-28, David Rowley wrote:
>
>> 0003: Is a tidy up patch to make the 'includes' field an array rather
>> than a string
> In this one, you can avoid turning one line into four with map,
>
> - $p->AddIncludeDir($pl_proj->{includes});
> + foreach my $inc (@{ $pl_proj->{includes} })
> + {
> + $p->AddIncludeDir($inc);
> + }
>
> Instead of that you can do something like this:
>
> + map { $p->AddIncludeDir($_); } @{$pl_proj->{includes}};
using map() for a side effect like this is generally frowned upon. See
<https://metacpan.org/pod/Perl::Critic::Policy::BuiltinFunctions::ProhibitVoidMap>
do { $p->AddIncludeDir($_); } foreach @{$pl_proj->{includes}};
would be an ok one-liner.
>
>> 0004: Adds code to check for duplicate references and libraries before
>> adding new ones of the same name to the project.
> I think using the return value of grep as a boolean is confusing. It
> seems more legible to compare to 0. So instead of this:
>
> + if (! grep { $_ eq $ref} @{ $self->{references} })
> + {
> + push @{ $self->{references} }, $ref;
> + }
>
> use something like:
>
> + if (grep { $_ eq $ref} @{ $self->{references} } == 0)
>
But I believe that's a widely used idiom :-)
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Reduce the number of special cases to build contrib modules on windows
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Jul-28, David Rowley wrote:
>
>> 0003: Is a tidy up patch to make the 'includes' field an array rather
>> than a string
>
> In this one, you can avoid turning one line into four with map,
>
> - $p->AddIncludeDir($pl_proj->{includes});
> + foreach my $inc (@{ $pl_proj->{includes} })
> + {
> + $p->AddIncludeDir($inc);
> + }
>
> Instead of that you can do something like this:
>
> + map { $p->AddIncludeDir($_); } @{$pl_proj->{includes}};
map (and grep) should never be used void context for side effects. Our
perlcritic policy doesn't currently forbid that, but it should (and
there is one violation of that in contrib/intarray). I'll submit a
patch for that separately.
The acceptable one-liner version would be a postfix for loop:
$p->AddIncludeDir($_) for @{$pl_proj->{includes}};
>> 0004: Adds code to check for duplicate references and libraries before
>> adding new ones of the same name to the project.
>
> I think using the return value of grep as a boolean is confusing. It
> seems more legible to compare to 0. So instead of this:
>
> + if (! grep { $_ eq $ref} @{ $self->{references} })
> + {
> + push @{ $self->{references} }, $ref;
> + }
>
> use something like:
>
> + if (grep { $_ eq $ref} @{ $self->{references} } == 0)
I disagree. Using grep in boolean context is perfectly idiomatic perl.
What would be more idiomatic is List::Util::any, but that's not availble
without upgrading List::Util from CPAN on Perls older than 5.20, so we
can't use that.
- ilmari
Re: Reduce the number of special cases to build contrib modules on windows
From
Alvaro Herrera
Date:
On 2021-Jul-27, Dagfinn Ilmari Mannsåker wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > + if (grep { $_ eq $ref} @{ $self->{references} } == 0)
>
> I disagree. Using grep in boolean context is perfectly idiomatic perl.
> What would be more idiomatic is List::Util::any, but that's not availble
> without upgrading List::Util from CPAN on Perls older than 5.20, so we
> can't use that.
I was wondering if instead of grepping the whole list for each addition
it would make sense to push always, and do a unique-ification step at
the end.
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
On Wed, 28 Jul 2021 at 02:33, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > David Rowley <dgrowleyml@gmail.com> writes: > > 0001: Removes an include directory for dblink. This appears like it's > > not needed. It was added in ee3b4188a (Jan 2010), but an earlier > > commit, 320c7eb8c (June 2008) seems to have made it pointless. It's > > still a mystery to me why ee3b4188a would have been required in the > > first place. > > FWIW, I poked around in the mailing list archives around that date > and couldn't find any supporting discussion. It does seem like it > shouldn't be needed, given that dblink's Makefile does no such thing. I think the reason is that src/backend/utils/Makefile symlinks fmgroids.h into src/include/utils. The copy you added in 320c7eb8c seems to be the MSVC build's equivalent of that. David
On Wed, 28 Jul 2021 at 01:44, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> I don't know anything about the MSVC build process, but I figured I
> could do a general Perl code review.
Thanks for looking at this. Perl review is very useful as it's
certainly not my native tongue, as you might have noticed.
> > --- a/src/tools/msvc/Mkvcbuild.pm
> > +++ b/src/tools/msvc/Mkvcbuild.pm
> […]
> > + # Process custom compiler flags
> > + if ($mf =~ /^PG_CPPFLAGS\s*=\s*(.*)$/mg || $mf =~ /^override\s*CPPFLAGS\s*(?:[\+\:])?=\s*(.*)$/mg)
> ^^^^^^^^^^^
> This is a very convoluted way of writing [+:]?
I've replaced the (?:[\+\:])? with [+:]? It's a bit of a blind
adjustment. I see that the resulting vcxproj files have not changed as
a result of that.
> > --- a/src/tools/msvc/Project.pm
> > +++ b/src/tools/msvc/Project.pm
> > @@ -58,7 +58,7 @@ sub AddFiles
> >
> > while (my $f = shift)
> > {
> > - $self->{files}->{ $dir . "/" . $f } = 1;
> > + AddFile($self, $dir . "/" . $f, 1);
>
> AddFile is a method, so should be called as $self->AddFile(…).
Adjusted thanks.
> > --- a/src/tools/msvc/Mkvcbuild.pm
> > +++ b/src/tools/msvc/Mkvcbuild.pm
> > @@ -36,16 +36,12 @@ my @unlink_on_exit;
> […]
> > + elsif ($flag =~ /^-I(.*)$/)
> > + {
> > + foreach my $proj (@projects)
> > + {
> > + if ($1 eq '$(libpq_srcdir)')
> > + {
> > + $proj->AddIncludeDir('src\interfaces\libpq');
> > + $proj->AddReference($libpq);
> > + }
> > + }
> > + }
>
> It would be better to do the if check outside the for loop.
Agreed.
> > --- a/src/tools/msvc/Project.pm
> > +++ b/src/tools/msvc/Project.pm
> > @@ -51,6 +51,16 @@ sub AddFile
> > return;
> > }
> >
> > +sub AddFileAndAdditionalFiles
> > +{
> > + my ($self, $filename) = @_;
> > + if (FindAndAddAdditionalFiles($self, $filename) != 1)
>
> Again, FindAndAddAdditionalFiles is a method and should be called as
> $self->FindAndAddAdditionalFiles($filename).
>
> > + {
> > + $self->{files}->{$filename} = 1;
> > + }
> > + return;
> > +}
Adjusted.
I'll send updated patches once I look at the other reviews.
David
On Wed, 28 Jul 2021 at 03:52, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>
> > On 2021-Jul-28, David Rowley wrote:
> >
> >> 0003: Is a tidy up patch to make the 'includes' field an array rather
> >> than a string
> >
> > In this one, you can avoid turning one line into four with map,
> >
> > - $p->AddIncludeDir($pl_proj->{includes});
> > + foreach my $inc (@{ $pl_proj->{includes} })
> > + {
> > + $p->AddIncludeDir($inc);
> > + }
> >
> > Instead of that you can do something like this:
> >
> > + map { $p->AddIncludeDir($_); } @{$pl_proj->{includes}};
>
> map (and grep) should never be used void context for side effects. Our
> perlcritic policy doesn't currently forbid that, but it should (and
> there is one violation of that in contrib/intarray). I'll submit a
> patch for that separately.
>
> The acceptable one-liner version would be a postfix for loop:
>
> $p->AddIncludeDir($_) for @{$pl_proj->{includes}};
I'm not sure if this is all just getting overly smart about it.
There's already a loop next to this doing:
foreach my $type_lib (@{ $type_proj->{libraries} })
{
$p->AddLibrary($type_lib);
}
I don't object to changing mine, if that's what people think who are
more familiar with Perl than I am, but I do think consistency is a
good thing. TBH, I kinda prefer the multi-line loop. I think most
people that look at these scripts are going to be primarily C coders,
so assuming each of the variations do the same job, then I'd rather
see us stick to the most C like version.
In the meantime, I'll just change it to $p->AddIncludeDir($_) for
@{$pl_proj->{includes}};. I just wanted to note my thoughts.
David
On Wed, 28 Jul 2021 at 04:01, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Jul-27, Dagfinn Ilmari Mannsåker wrote:
>
> > Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>
> > > + if (grep { $_ eq $ref} @{ $self->{references} } == 0)
> >
> > I disagree. Using grep in boolean context is perfectly idiomatic perl.
> > What would be more idiomatic is List::Util::any, but that's not availble
> > without upgrading List::Util from CPAN on Perls older than 5.20, so we
> > can't use that.
>
> I was wondering if instead of grepping the whole list for each addition
> it would make sense to push always, and do a unique-ification step at
> the end.
I see [1] has some thoughts on this, I don't think performance will
matter much here though. I think the order of the final array is
likely more important. I didn't test, but I imagine using one of
those hash solutions might end up having the array elements in some
hashtable like order.
I'm not quite sure if I can tell here if it's ok to leave the grep
as-is or if I should be changing it to:
if (grep { $_ eq $ref} @{ $self->{references} } == 0)
David
[1] https://www.oreilly.com/library/view/perl-cookbook/1565922433/ch04s07.html
On Wed, 28 Jul 2021 at 03:52, Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > I think using the return value of grep as a boolean is confusing. It
> > seems more legible to compare to 0. So instead of this:
> >
> > + if (! grep { $_ eq $ref} @{ $self->{references} })
> > + {
> > + push @{ $self->{references} }, $ref;
> > + }
> >
> > use something like:
> >
> > + if (grep { $_ eq $ref} @{ $self->{references} } == 0)
>
> I disagree. Using grep in boolean context is perfectly idiomatic perl.
> What would be more idiomatic is List::Util::any, but that's not availble
> without upgrading List::Util from CPAN on Perls older than 5.20, so we
> can't use that.
Ok, if the grep stuff is ok as is with the boolean comparison then I'd
say 0002 and 0003 of the attached are ok to go.
I pushed the v9 0001 and 0005 patch after adjusting the AddFile($self,
...) to become $self->AddFile(...)
I've adjusted the attached 0001 patch (previously 0002) to define
LOWER_NODE in ltree.h as mentioned by Tom.
0004 still needs work.
Thanks for all the reviews.
David
Attachment
On Thu, 29 Jul 2021 at 00:05, David Rowley <dgrowleyml@gmail.com> wrote: > I pushed the v9 0001 and 0005 patch after adjusting the AddFile($self, > ...) to become $self->AddFile(...) I pushed all the patches, apart from the 0004 patch. One weird thing I noticed with the -D define patch (245de4845) and the LOWER_NODE adjustment is that in crc32.c we do: #ifdef LOWER_NODE #include <ctype.h> #define TOLOWER(x) tolower((unsigned char) (x)) #else #define TOLOWER(x) (x) #endif meaning when LOWER_NODE Is defined that the CRC is hashed all in lower case, effectively making it case-insensitive. Whereas in ltree.h we do: #ifdef LOWER_NODE #define FLG_CANLOOKSIGN(x) ( ( (x) & ( LQL_NOT | LVAR_ANYEND | LVAR_SUBLEXEME ) ) == 0 ) #else #define FLG_CANLOOKSIGN(x) ( ( (x) & ( LQL_NOT | LVAR_ANYEND | LVAR_SUBLEXEME | LVAR_INCASE ) ) == 0 ) #endif So effectively if LOWER_NODE is defined then we *don't* pass the LVAR_INCASE which makes the comparisons case-sensitive! I've never used ltree before, so might just be misunderstanding something, but this does smell quite buggy to me. However, I just made it work how it always has worked. > 0004 still needs work. I adjusted this one so that it does the right thing for all the existing .l and .y files and correctly adds the relevant .c file when required, but to be honest, I just made this work by checking that the each of the vcxproj files match before and after the change. There is code that parses the likes of the following in the cube contrib module's Makefile: # cubescan is compiled as part of cubeparse cubeparse.o: cubescan.c Here, since cubescan.c is not added to the project files for compilation, I made that just call the: AddDependantFiles function, which just searches for .l and .y files that exist with the same name, but does not add the actual file passed to the function. This means that we add cubescan.l to the project but not cubscan.c. This is different from what happens with ecpg with pgc.l. We also add pgc.c to the project in that case because it's mentioned in OBJS in src/interfaces/ecpg/preproc/Makefile. The only change in the outputted vcxproj files that the attached produces is an order change in the AdditionalDependencies of libpq_pipeline.vcxproj I also managed to remove libpq_pipeline from contrib_uselibpgport and contrib_uselibpgcommon. The parsing for SHLIB_LINK_INTERNAL and PG_LIBS_INTERNAL only allowed for = not +=. Does anyone have any thoughts on where we should draw the line on parsing Makefiles? I'm a little worried that I'm adding pasing just for exactly how the Makefiles are today and that it could easily be broken if something is adjusted later. I'm not all that skilled with make, so I'm struggling to judge this for myself. David
Attachment
On Fri, 30 Jul 2021 at 15:05, David Rowley <dgrowleyml@gmail.com> wrote: > Does anyone have any thoughts on where we should draw the line on > parsing Makefiles? I'm a little worried that I'm adding pasing just > for exactly how the Makefiles are today and that it could easily be > broken if something is adjusted later. I'm not all that skilled with > make, so I'm struggling to judge this for myself. After thinking about this some more, I think since we're never going to make these Perl scripts do everything that make can do, that if the parsing that's being added seems reasonable and works for what we have today, there I don't think there is much reason not to just go with this. The v10 patch I attached previously output almost identical *.vcxproj files. The only change was in libpq_pipeline.vcxproj where the order of the AdditionalDependencies changed to have ws2_32.lib first rather than somewhere in the middle. I've now pushed the final patch in this series. Thank you to everyone who looked at one or more of these patches. David