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

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