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