Subject: meson: pgxs Makefile.global differences

From: Andrew Dunstan <[email-masked]>
Date: Thu, 17 Aug 2023 15:32:40 -0400

I started digging into a warning I noticed on my FDW builds where 
Postgres is built with meson, e.g. 
<https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2023-08-16%2018%3A37%3A25&stg=FileTextArrayFDW-build> 
which has this:

cc1: warning: ‘-Wformat-security’ ignored without ‘-Wformat’ 
[-Wformat-security]

I found that the pgxs Makefile.global built under meson is a bit 
different. On debug builds for both this is what I get on HEAD (meson) 
and REL_15_STABLE (autoconf), stripped of the current components:

          HEAD: CFLAGS =-Wshadow=compatible-local
REL_15_STABLE: CFLAGS =-Wall  -g

The warning is apparently due to the missing -Wall.

Shouldn't we be aiming for pretty much identical settings?

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

---

From: "Tristan Partin" <[email-masked]>
Date: Thu, 17 Aug 2023 14:45:54 -0500

> Shouldn't we be aiming for pretty much identical settings?

I agree that they should be identical. The meson bild should definitely 
be aiming for 100% compatibility for the Makefile.global.

-- 
Tristan Partin
Neon (https://neon.tech)

---

From: Andres Freund <[email-masked]>
Date: Thu, 17 Aug 2023 13:51:42 -0700

Hi,

On 2023-08-17 14:45:54 -0500, Tristan Partin wrote:
> > Postgres is built with meson, e.g. <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2023-08-16%2018%3A37%3A25&stg=FileTextArrayFDW-build>
> > which has this:
> >

I assume "current" means the flags that are present in both cases?

> > Shouldn't we be aiming for pretty much identical settings?

The difference for -Wshadow=compatible-local is due to changes between 15 and
HEAD.

We're indeed not adding -Wall right now (the warning level is handled by
meson, so it doesn't show up in our cflags right now).

> I agree that they should be identical. The meson bild should definitely be
> aiming for 100% compatibility for the Makefile.global.

I don't think that's feasible. It was a fair bit of work to get the most
important contents to match, while skipping lots of things that are primarily
relevant for building the server (which isn't relevant for pgxs).

That said, in this specific case, I agree, we should likely emit -Wall to
Makefile.global in meson as well.

Greetings,

Andres Freund

PS: I don't have [email-masked] , just .de :)

---

From: "Tristan Partin" <[email-masked]>
Date: Thu, 17 Aug 2023 15:56:37 -0500

> PS: I don't have [email-masked] , just .de :)

Fat fingered a "v" somehow.

-- 
Tristan Partin
Neon (https://neon.tech)

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 21 Aug 2023 11:33:45 -0400

On 2023-08-17 Th 16:51, Andres Freund wrote:
>>> Postgres is built with meson, e.g.<https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2023-08-16%2018%3A37%3A25&stg=FileTextArrayFDW-build>
>>> which has this:
>>>

Where should we do that? And how about the -g that's also missing for 
debug-enabled builds?

cheers

andrew

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 21 Aug 2023 17:43:48 +0200

> Where should we do that? And how about the -g that's also missing for 

I think it's the options in these two tables that meson handles 
internally and that we should explicitly reproduce for Makefile.global:

https://mesonbuild.com/Builtin-options.html#details-for-buildtype
https://mesonbuild.com/Builtin-options.html#details-for-warning_level

---

From: "Tristan Partin" <[email-masked]>
Date: Mon, 21 Aug 2023 10:48:23 -0500

> Where should we do that? And how about the -g that's also missing for 

Look in src/makefiles/meson.build. You will see a line like
'CFLAGS': var_cflags. You probably want to do something like:

	pgxs_cflags = var_cflags + cc.get_supported_arguments('-Wxxx')
	if get_option('debug')
		# Populate for debug flags that aren't -g
		debug_flags = {}

		pgxs_cflags += debug_flags.get(cc.get_id(), 
			cc.get_supported_arguments('-g')
	endif

	...
	CFLAGS: pgxs_cflags,
	...

-- 
Tristan Partin
Neon (https://neon.tech)