Feedback-ID: id4a34324:Fastmail
Date: Wed, 17 Aug 2022 14:53:17 -0700
From: Andres Freund <andres@anarazel.de>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: pgsql-hackers@postgresql.org, samay sharma <smilingsamay@gmail.com>,
	Nazir Bilal Yavuz <byavuz81@gmail.com>,
	Andrew Dunstan <andrew@dunslane.net>
Subject: Re: [RFC] building postgres with meson - v11
Message-ID: <20220817215317.poeofidf7o7dy6hy@awork3.anarazel.de>
References: <20211012083721.hvixq4pnh2pixr3j@alap3.anarazel.de>
 <20220811002012.ju3rrz47i2e5tdha@awork3.anarazel.de>
 <e0c44fb2-8b66-a4b9-b274-7ed3a1a0ab74@enterprisedb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e0c44fb2-8b66-a4b9-b274-7ed3a1a0ab74@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220817215317.poeofidf7o7dy6hy%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2022-08-17 15:50:23 +0200, Peter Eisentraut wrote:
> - There are various references to "pch" (pre-compiled headers).  Is
>   there more discussion anywhere about this?  I don't know what this
>   would entail or whether there are any drawbacks to be aware of.  The
>   new *_pch.h files don't have any comments.  Maybe this should be a
>   separate patch later.

It's mainly to make windows builds a bit slower. I've no objection to
separating this out.


> - About relativize_shared_library_references: We have had several
>   patches over the years for working around SIP stuff, and some of
>   them did essentially this, but we decided not to go ahead with them.
>   We could revisit that, but it should be a separate patch, not mixed
>   in with this.

The prior approaches all had issues because they didn't support relative
references IIRC (and thus broke being able to relocate the installation),
which this does.

I just found it very annoying to work on macs without this. And there were at
least two "bug" reports of testers of the meson branch that were just due to
SIP.

I'm ok with splitting it out, but I also think it's a lower risk opportunity
to test that this works.


> - postgresql-extension.pc: Similarly, this ought to be a separate
>   patch.  If we want people to use this, we'll need it in the makefile
>   build system anyway.

Makes sense. I'd like to keep it in the same patch for a short while longer,
to deduplicate some of the code, but then will split it out.


> - -DFRONTEND is used somewhat differently from the makefiles.  For
>    example, meson sets -DFRONTEND for pg_controldata, but the
>    makefiles don't.  Conversely, the makefiles set -DFRONTEND for
>    ecpglib, but meson does not.  This should be checked again to make
>    sure it all matches up.

Yes, should sync that up.

FWIW, meson does add -DFRONTEND for ecpglib. There were a few places that did
add it twice, I'll push a cleanup of that in a bit.


> - Option name spelling should be make consistent about underscores
>   versus hyphens.  Built-in meson options use underscores, so we
>   should make the user-defined ones like that as well (some already
>   do).  (wal-blocksize krb-srvnam system-tzdata tap-tests bsd-auth)

No objection.


> - I have found the variable name "cdata" for configuration_data() to
>   be less than clear.  I see some GNOME projects to it that way, is
>   that where it's from?  systemd uses "conf", maybe that's better.

I don't know where it's from - I don't think I ever looked at gnome
buildsystem stuff. It seems to be the obvious abbreviation for
configuration_data()...  I don't object to conf, but it's not a clear
improvement to me.


> - In the top-level meson.build, the "renaming" of the Windows system
>   name
>
>       host_system = host_machine.system() == 'windows' ? 'win32' :
> host_machine.system()
>       build_system = build_machine.system() == 'windows' ? 'win32' :
> build_machine.system()
>
>   seems unnecessary to me.  Why not stick with the provided names?

Because right now we also use it for things like choosing the "source" for
pg_config_os.h (i.e. include/port/{darwin,linux,win32,..}.h). And it seemed
easier to just have one variable name for all of it.


> - The c99_test ought to be not needed if the c_std project option is
>   used.  Was there a problem with that?

We don't want to force -std=c99 when not necessary, I think. We sometimes use
features from newer (and from gnu) language versions after testing
availability, and if we hardcode the version those will either fail or elicit
warnings.


> - Is there a way to split up the top-level meson.build somehow?  Maybe
>   just throw some stuff into included files?  This might get out of
>   hand at some point.

We can put stuff into config/meson.build or such. But I don't think it's
clearly warranted at this point.


> - The PG_SYSROOT detection gives different results.  On my system,
>   configure produces
>
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk,
>   meson produces
>
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk.
>   src/template/darwin goes out of its way to get a version-specific
>   result, so we need to carry that over somehow.  (The difference does
>   result in differences in the built binaries.)

TBH, I don't really understand the SYSROOT stuff all that well, never having
used a mac in anger (well, only in anger, but ...).

What do you think about extracting the relevant portion of src/template/darwin
into a dedicated shell script that gets called by both?


> Then, some patches from me:
>
> 0001-Change-shared-library-installation-naming-on-macOS.patch
>
> This changes the makefiles to make the shared library file naming on
> macOS match what meson produces.  I don't know what the situation is
> on other platforms.

No opinion on the matter. Seems best to apply separately if we want to?


> 0002-meson-Fix-installation-name-of-libpgfeutils.patch
>
> This was presumably an accidental mistake.

Yes, merged.


> 0003-meson-Libraries-need-to-be-built-with-DSO_MAJOR_VERS.patch
>
> This is needed to make NLS work for the libraries.

Oh, huh. Yes, merged.


> 0004-meson-Add-darwin_versions-argument-for-libraries.patch
>
> This is to make the output match what Makefile.shlib produces.

:/, merged. Would be good to clean up at some point.


> 0005-meson-Fix-link-order-of-support-libraries.patch
> 0006-meson-Make-link-order-of-external-libraries-match-ma.patch
> 0007-WIP-meson-Make-link-order-of-object-files-match-make.patch
>
> I have analyzed the produced binaries between both build systems to
> make sure they match.  If we link the files and libraries in different
> orders, that becomes difficult.  So this fixes this up a bit.  0005 is
> needed for correctness in general, I think.

Makes sense.


> 0006 is mostly cosmetic.  You probably wanted to make the library order
> alphabetical in the meson files, which I'd support, but then we should
> change the makefiles to match.

Trying to match makefile order doesn't seem like a good plan, given that it's
effectively random, and might change depending on dependencies of linked to
libraries etc.

Isn't the use of AC_CHECK_LIB for at least lz4, zstd completely bogus? We get
whether they're available via pkg-config, but then completely ignore the
linker flag for the library name. The comment says:
  # We only care about -I, -D, and -L switches;
  # note that -llz4 will be added by AC_CHECK_LIB below.
but without any further explanation. This seems to be from 4d399a6fbeb.


The repetition of lz4, zstd in pg_rewind, pg_waldump and backend makes me
wonder if we should put them in a xlogreader_deps or such. It's otherwise not
obvious why pg_rewind, pg_waldump need lz4/zstd.


> Similarly, 0007, which is clearly a bit
> messy at the moment, but we should try to sort that out either in the old or
> the new build files.

I am against trying to maintain bug-for-bug compatibility on filename
ordering. But obviously ok with fixing the ordering to make sense on both
sides.

What was your decision point about when to adjust makefile ordering and when
meson ordering?


> And finally some comments on your patches:

Any comment on the pg_regress_ecpg commit? I'd like to get that out of the
way, and it seems considerably cleaner than the hackery we do right now to
make VPATH builds work.


> meson: prereq: Don't add HAVE_LDAP_H HAVE_WINLDAP_H to pg_config.h.
>
> This can go ahead.
>
> meson: prereq: fix warning compat_informix/rnull.pgc with msvc
>
> -   $float f = 3.71;
> +   $float f = (float) 3.71;
>
> This could use float literals like
>
> +   $float f = 3.71f;

I tried that first, but it fails:
../src/interfaces/ecpg/test/compat_informix/rnull.pgc:19: ERROR: trailing junk after numeric literal

Should have noted that. I don't feel like fixing ecpg's parser etc...


Greetings,

Andres Freund