Обсуждение: Reduce the number of special cases to build contrib modules on windows

Поиск
Список
Период
Сортировка

Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
Andres Freund
Дата:
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



Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
Alvaro Herrera
Дата:
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 ... 



Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
Michael Paquier
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
Michael Paquier
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
Michael Paquier
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
Alvaro Herrera
Дата:
> 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

От
Andrew Dunstan
Дата:
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




Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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



Re: Reduce the number of special cases to build contrib modules on windows

От
vignesh C
Дата:
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



Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Дата:
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



Re: Reduce the number of special cases to build contrib modules on windows

От
Tom Lane
Дата:
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

От
Alvaro Herrera
Дата:
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

От
Andrew Dunstan
Дата:
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

От
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Дата:
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

От
Alvaro Herrera
Дата:
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/



Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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



Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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



Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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



Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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



Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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

Вложения

Re: Reduce the number of special cases to build contrib modules on windows

От
David Rowley
Дата:
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