Thread: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

[PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Christoph Berg
Date:
While working on making extension modules built reproducibly, I
noticed that extra flags passed via COPT (notably -ffile-prefix-map)
do not get added to CXXFLAGS.

This causes postgresql-hll to still store the full build path in the
.so file built:
https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/diffoscope-results/postgresql-hll.html

Christoph
-- 
Senior Berater, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Andres Freund
Date:
Hi,

On 2018-11-13 11:40:05 +0100, Christoph Berg wrote:
> While working on making extension modules built reproducibly, I
> noticed that extra flags passed via COPT (notably -ffile-prefix-map)
> do not get added to CXXFLAGS.

PROFILE I can see, but COPT I'm less sure. The name suggests it's about
C not C++. How about adding CXXOPT?

Secondary question:  Why aren't you using CFLAGS/CXXFLAGS for this?

Greetings,

Andres Freund


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Tom Lane
Date:
Andres Freund <andres@anarazel.de> writes:
> On 2018-11-13 11:40:05 +0100, Christoph Berg wrote:
>> While working on making extension modules built reproducibly, I
>> noticed that extra flags passed via COPT (notably -ffile-prefix-map)
>> do not get added to CXXFLAGS.

> PROFILE I can see, but COPT I'm less sure. The name suggests it's about
> C not C++. How about adding CXXOPT?

> Secondary question:  Why aren't you using CFLAGS/CXXFLAGS for this?

COPT (and PROFILE) are handy for injecting additional flags manually;
they're even documented for that (cf installation.sgml, around line
1550).

I agree that CXXOPT would be a better idea than COPT for this.
Not sure about PROFILE.  But we could inject the latter into both
flag sets and then document that if that's not what you want, use COPT.

            regards, tom lane


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Andres Freund
Date:
Hi,

On 2018-11-13 17:27:17 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2018-11-13 11:40:05 +0100, Christoph Berg wrote:
> >> While working on making extension modules built reproducibly, I
> >> noticed that extra flags passed via COPT (notably -ffile-prefix-map)
> >> do not get added to CXXFLAGS.
> 
> > PROFILE I can see, but COPT I'm less sure. The name suggests it's about
> > C not C++. How about adding CXXOPT?
> 
> > Secondary question:  Why aren't you using CFLAGS/CXXFLAGS for this?
> 
> COPT (and PROFILE) are handy for injecting additional flags manually;
> they're even documented for that (cf installation.sgml, around line
> 1550).

> I agree that CXXOPT would be a better idea than COPT for this.

Yea, I agree that we want CXX equivalent of the feature, especially for
-Werror.  I'm just not sure that's the best fit for the debian build
issue Christoph ran into. I guess the goal is to *not* include the
-ffile-prefix-map in the CFLAGS for extensions, unless those are also
built via debian machinery?


> Not sure about PROFILE.  But we could inject the latter into both
> flag sets and then document that if that's not what you want, use
> COPT.

That seems reasonable to me too.

Greetings,

Andres Freund


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Christoph Berg
Date:
(Sorry for the delayed response here.)

Re: Andres Freund 2018-11-13 <20181113223330.2ql7tg33hhh6husf@alap3.anarazel.de>
> > >> While working on making extension modules built reproducibly, I
> > >> noticed that extra flags passed via COPT (notably -ffile-prefix-map)
> > >> do not get added to CXXFLAGS.
> > 
> > > PROFILE I can see, but COPT I'm less sure. The name suggests it's about
> > > C not C++. How about adding CXXOPT?

Any of that would work, I was adding it to both because both were the
same so far.

> > > Secondary question:  Why aren't you using CFLAGS/CXXFLAGS for this?

The context here is that we want to use the *FLAGS from pg_config for
compiling PG extension packages, but add additional *FLAGS from the
extension build environment. Merging the pg_config CFLAGS with the
environment CFLAGS seemed hard/weird/error-prone, so what we are doing
now is https://salsa.debian.org/postgresql/postgresql-common/blob/master/pg_buildext#L53-55

    if [ "${CFLAGS:-}" ]; then
        export COPT="$CFLAGS"
    fi

Which works nicely.

> > COPT (and PROFILE) are handy for injecting additional flags manually;
> > they're even documented for that (cf installation.sgml, around line
> > 1550).
> 
> > I agree that CXXOPT would be a better idea than COPT for this.
> 
> Yea, I agree that we want CXX equivalent of the feature, especially for
> -Werror.  I'm just not sure that's the best fit for the debian build
> issue Christoph ran into. I guess the goal is to *not* include the
> -ffile-prefix-map in the CFLAGS for extensions, unless those are also
> built via debian machinery?

-ffile-prefix-map (or actually still -fdebug-prefix-map, -ffile-p-m is
WIP) is the reason for the extra flags, yes. We can't use pg_config's
version here because the build path for PG is not the build path for
the extension module.

> > Not sure about PROFILE.  But we could inject the latter into both
> > flag sets and then document that if that's not what you want, use
> > COPT.
> 
> That seems reasonable to me too.

Updated patch:

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 956fd274cd..42bdf9f75c 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -674,8 +674,14 @@ ifdef COPT
    LDFLAGS += $(COPT)
 endif
 
+ifdef CXXOPT
+   CXXFLAGS += $(CXXOPT)
+   LDFLAGS += $(CXXOPT)
+endif
+
 ifdef PROFILE
    CFLAGS += $(PROFILE)
+   CXXFLAGS += $(PROFILE)
    LDFLAGS += $(PROFILE)
 endif
 

I didn't patch the documentation yet because I wasn't sure if the
LDFLAGS handling with CXXOPT makes sense.

Christoph
-- 
Senior Berater, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Peter Eisentraut
Date:
On 21/11/2018 14:28, Christoph Berg wrote:
> The context here is that we want to use the *FLAGS from pg_config for
> compiling PG extension packages, but add additional *FLAGS from the
> extension build environment. Merging the pg_config CFLAGS with the
> environment CFLAGS seemed hard/weird/error-prone, so what we are doing
> now is https://salsa.debian.org/postgresql/postgresql-common/blob/master/pg_buildext#L53-55
> 
>     if [ "${CFLAGS:-}" ]; then
>         export COPT="$CFLAGS"
>     fi

Note that pgxs supports PG_CPPFLAGS for adding custom pieces to CPPFLAGS
in a safe way.  Maybe we should add an equivalent for CFLAGS?  It's just
that it hasn't been needed so far.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Fri, Jan 04, 2019 at 11:41:15AM +0100, Peter Eisentraut wrote:
> Note that pgxs supports PG_CPPFLAGS for adding custom pieces to CPPFLAGS
> in a safe way.  Maybe we should add an equivalent for CFLAGS?  It's just
> that it hasn't been needed so far.

+1.  Wouldn't it make sense to also have PG_LDFLAGS?  In some stuff I
maintain, I have been abusing of PG_CPPFLAGS to pass some flags, which
is not really right.  We also have contrib/passwordcheck/Makefile
abuse of it to set -DUSE_CRACKLIB..
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Christoph Berg
Date:
Re: Michael Paquier 2019-01-04 <20190104133305.GG2067@paquier.xyz>
> On Fri, Jan 04, 2019 at 11:41:15AM +0100, Peter Eisentraut wrote:
> > Note that pgxs supports PG_CPPFLAGS for adding custom pieces to CPPFLAGS
> > in a safe way.  Maybe we should add an equivalent for CFLAGS?  It's just
> > that it hasn't been needed so far.
> 
> +1.  Wouldn't it make sense to also have PG_LDFLAGS?  In some stuff I
> maintain, I have been abusing of PG_CPPFLAGS to pass some flags, which
> is not really right.  We also have contrib/passwordcheck/Makefile
> abuse of it to set -DUSE_CRACKLIB..

I'll buy whatever version that works. I'm using CFLAGS at the moment,
but could easily switch to other variables.

Note that the missing bit here is the CXX part, adding PG_CFLAGS alone
wouldn't help.

Updated patch attached.

Christoph
-- 
Senior Berater, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Christoph Berg
Date:
Re: To Michael Paquier 2019-01-07 <20190107091734.GA1582@msg.credativ.de>
> Updated patch attached.

Here's another revision that doesn't add an extra CXXOPT variable but
just extends CXXFLAGS whenever COPT or PROFILE are set, which seems
more usable.

It also updates the documentation.

Christoph
-- 
Senior Berater, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Mon, Jan 07, 2019 at 10:32:20AM +0100, Christoph Berg wrote:
> Here's another revision that doesn't add an extra CXXOPT variable but
> just extends CXXFLAGS whenever COPT or PROFILE are set, which seems
> more usable.
>
> It also updates the documentation.

The documentation is not fully updated.  It still lacks the
description for each new field.
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Andres Freund
Date:
Hi,

On 2019-01-07 10:32:20 +0100, Christoph Berg wrote:
> Re: To Michael Paquier 2019-01-07 <20190107091734.GA1582@msg.credativ.de>
> > Updated patch attached.
> 
> Here's another revision that doesn't add an extra CXXOPT variable but
> just extends CXXFLAGS whenever COPT or PROFILE are set, which seems
> more usable.

Why does that seem more usable? How's that supposed to be used for flags
that aren't valid for both languages?

Greetings,

Andres Freund


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Christoph Berg
Date:
Re: Andres Freund 2019-01-08 <20190108011837.n4mx7dadvojv2x55@alap3.anarazel.de>
> > Here's another revision that doesn't add an extra CXXOPT variable but
> > just extends CXXFLAGS whenever COPT or PROFILE are set, which seems
> > more usable.
> 
> Why does that seem more usable? How's that supposed to be used for flags
> that aren't valid for both languages?

The existing COPT and PROFILE are already catch-all type flags that
add to CFLAGS and LDFLAGS. Possibly we should leave those alone and
only add PG_CXXFLAGS and PG_LDFLAGS?

Christoph
-- 
Senior Berater, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Tue, Jan 08, 2019 at 09:09:56AM +0100, Christoph Berg wrote:
> Re: Andres Freund 2019-01-08 <20190108011837.n4mx7dadvojv2x55@alap3.anarazel.de>
>>> Here's another revision that doesn't add an extra CXXOPT variable but
>>> just extends CXXFLAGS whenever COPT or PROFILE are set, which seems
>>> more usable.
>>
>> Why does that seem more usable? How's that supposed to be used for flags
>> that aren't valid for both languages?
>
> The existing COPT and PROFILE are already catch-all type flags that
> add to CFLAGS and LDFLAGS. Possibly we should leave those alone and
> only add PG_CXXFLAGS and PG_LDFLAGS?

Personally I see pgxs as something completely different than what COPT
and PROFILE are as we are talking about two different facilities: one
which is part of the core installation, and the other which can be
used for extension modules, so having PG_CFLAGS, PG_CXXFLAGS and
PG_LDFLAGS, but leaving CXXFLAGS out of COPT and PROFILE looks like
the better long-term move in terms of pluggability.  My 2c.
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Thu, Jan 17, 2019 at 02:55:39PM +0900, Michael Paquier wrote:
> Personally I see pgxs as something completely different than what COPT
> and PROFILE are as we are talking about two different facilities: one
> which is part of the core installation, and the other which can be
> used for extension modules, so having PG_CFLAGS, PG_CXXFLAGS and
> PG_LDFLAGS, but leaving CXXFLAGS out of COPT and PROFILE looks like
> the better long-term move in terms of pluggability.  My 2c.

It's been a couple of days since this message, and while my opinion
has not really changed, there are many other opinions.  So perhaps we
could reduce the proposal to a strict minimum and find an agreement
for the options that we think are absolutely worth adding?  Even if we
cannot agree on what COPT of PROFILE should do more, perhaps we could
still agree with only a portion of the flags we think are worth it?
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Andres Freund
Date:
On 2019-01-22 15:26:21 +0900, Michael Paquier wrote:
> On Thu, Jan 17, 2019 at 02:55:39PM +0900, Michael Paquier wrote:
> > Personally I see pgxs as something completely different than what COPT
> > and PROFILE are as we are talking about two different facilities: one
> > which is part of the core installation, and the other which can be
> > used for extension modules, so having PG_CFLAGS, PG_CXXFLAGS and
> > PG_LDFLAGS, but leaving CXXFLAGS out of COPT and PROFILE looks like
> > the better long-term move in terms of pluggability.  My 2c.
> 
> It's been a couple of days since this message, and while my opinion
> has not really changed, there are many other opinions.  So perhaps we
> could reduce the proposal to a strict minimum and find an agreement
> for the options that we think are absolutely worth adding?  Even if we
> cannot agree on what COPT of PROFILE should do more, perhaps we could
> still agree with only a portion of the flags we think are worth it?

I think its plain wrong to add COPT to CXXFLAGS. Re PROFILE I'm on the
fence.  I personally think the pgxs stuff is a bit separate, and I'm
doubtful we ought to backpatch that.  I'm basically planning to apply
https://www.postgresql.org/message-id/20190107091734.GA1582%40msg.credativ.de
to 11-, minus the PGXS stuff. If we want that, we ought to apply it to
master only IMO.

Greetings,

Andres Freund


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Alvaro Herrera
Date:
On 2019-Jan-22, Andres Freund wrote:

> I think its plain wrong to add COPT to CXXFLAGS. Re PROFILE I'm on the
> fence.  I personally think the pgxs stuff is a bit separate, and I'm
> doubtful we ought to backpatch that.  I'm basically planning to apply
> https://www.postgresql.org/message-id/20190107091734.GA1582%40msg.credativ.de
> to 11-, minus the PGXS stuff. If we want that, we ought to apply it to
> master only IMO.

I don't understand why you don't want to backpatch the PGXS bits.  Is
there something working today that would be broken by it?  I think
you're worried about places that invoke makefiles with PG_CXXFLAGS set
and expecting the value not to be propagated.  Is that a scenario we
need to worry about?

The patch neglects to update extend.sgml with the new pgxs variable,
though.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Andres Freund
Date:
Hi,

On 2019-01-22 17:10:58 -0300, Alvaro Herrera wrote:
> On 2019-Jan-22, Andres Freund wrote:
> 
> > I think its plain wrong to add COPT to CXXFLAGS. Re PROFILE I'm on the
> > fence.  I personally think the pgxs stuff is a bit separate, and I'm
> > doubtful we ought to backpatch that.  I'm basically planning to apply
> > https://www.postgresql.org/message-id/20190107091734.GA1582%40msg.credativ.de
> > to 11-, minus the PGXS stuff. If we want that, we ought to apply it to
> > master only IMO.
> 
> I don't understand why you don't want to backpatch the PGXS bits.

Largely because I think it's an independent patch from the CXXOPT need
from Christopher / Debian packaging.  It's a larger patch, that needs
more docs etc.  If whoever applies that wants to backpatch it - I'm not
going to protest, I just wouldn't myself, unless somebody pipes up that
it'd help them.

Greetings,

Andres Freund


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Alvaro Herrera
Date:
Hello

On 2019-Jan-22, Andres Freund wrote:

> On 2019-01-22 17:10:58 -0300, Alvaro Herrera wrote:

> > I don't understand why you don't want to backpatch the PGXS bits.
> 
> Largely because I think it's an independent patch from the CXXOPT need
> from Christopher / Debian packaging.  It's a larger patch, that needs
> more docs etc.  If whoever applies that wants to backpatch it - I'm not
> going to protest, I just wouldn't myself, unless somebody pipes up that
> it'd help them.

Ah, I see.  No arguments against.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Tue, Jan 22, 2019 at 06:11:23PM -0300, Alvaro Herrera wrote:
> On 2019-Jan-22, Andres Freund wrote:
>> Largely because I think it's an independent patch from the CXXOPT need
>> from Christopher / Debian packaging.  It's a larger patch, that needs
>> more docs etc.  If whoever applies that wants to backpatch it - I'm not
>> going to protest, I just wouldn't myself, unless somebody pipes up that
>> it'd help them.
>
> Ah, I see.  No arguments against.

Thanks Andres and Alvaro for the input.  No issues with the way of
Andres proposes.

The new PGXS flags would be I think useful to make sure
that things for CFLAGS and LDFLAGS get propagated without having to
hijack the original flags, so I can handle that part.  Which one would
be wanted though?
- PG_CXXFLAGS
- PG_LDFLAGS
- PG_CFLAGS

I'd see value in all of them, still everybody has likely a different
opinion, so I would not mind discarding the ones are not thought as
that much useful.  New PGXS infrastructure usually finds only its way
on HEAD, so I'd rather not back-patch that part.  No issues with the
back-patch portion for CXXOPT from me as that helps Debian.

Thanks,
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Christoph Berg
Date:
Re: Michael Paquier 2019-01-23 <20190123004722.GE3873@paquier.xyz>
> >> Largely because I think it's an independent patch from the CXXOPT need
> >> from Christopher / Debian packaging.  It's a larger patch, that needs
> >> more docs etc.  If whoever applies that wants to backpatch it - I'm not
> >> going to protest, I just wouldn't myself, unless somebody pipes up that
> >> it'd help them.
> > 
> > Ah, I see.  No arguments against.

Fwiw I'm not attached to using COPT and friends, I just happend to
pick these because that was one way that worked. With what I've
learned now, PG_*FLAGS is the better approach.

> The new PGXS flags would be I think useful to make sure
> that things for CFLAGS and LDFLAGS get propagated without having to
> hijack the original flags, so I can handle that part.  Which one would
> be wanted though?
> - PG_CXXFLAGS
> - PG_LDFLAGS
> - PG_CFLAGS
> 
> I'd see value in all of them, still everybody has likely a different
> opinion, so I would not mind discarding the ones are not thought as
> that much useful.  New PGXS infrastructure usually finds only its way
> on HEAD, so I'd rather not back-patch that part.  No issues with the
> back-patch portion for CXXOPT from me as that helps Debian.

The attached patch adds these three.

Re backpatching, I would at least need them in PG11 because that's
what is going to be released with Debian buster. An official backpatch
to all supported versions would be nice, but I could also sneak in
that change into the Debian packages without breaking anything.

Christoph
-- 
Senior Berater, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Tue, Jan 29, 2019 at 04:18:46PM +0100, Christoph Berg wrote:
> Re backpatching, I would at least need them in PG11 because that's
> what is going to be released with Debian buster. An official backpatch
> to all supported versions would be nice, but I could also sneak in
> that change into the Debian packages without breaking anything.

The change is non-invasive, so if that helps to make packaging easier
and more consistent as a whole, then I could do the back-patch.
Except if somebody has an objection?
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Andres Freund
Date:
Hi,

On 2019-01-29 16:18:46 +0100, Christoph Berg wrote:
> Re: Michael Paquier 2019-01-23 <20190123004722.GE3873@paquier.xyz>
> > >> Largely because I think it's an independent patch from the CXXOPT need
> > >> from Christopher / Debian packaging.  It's a larger patch, that needs
> > >> more docs etc.  If whoever applies that wants to backpatch it - I'm not
> > >> going to protest, I just wouldn't myself, unless somebody pipes up that
> > >> it'd help them.
> > > 
> > > Ah, I see.  No arguments against.
> 
> Fwiw I'm not attached to using COPT and friends, I just happend to
> pick these because that was one way that worked. With what I've
> learned now, PG_*FLAGS is the better approach.

I'm confused - that doesn't allow to inject flags to all in-core built
files? So how does that fix your problem fully? Say
e.g. llvmjit_inline.cpp won't get the flag, and thus not be
reproducible?

Greetings,

Andres Freund


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Christoph Berg
Date:
Re: Andres Freund 2019-01-30 <20190130015127.hciz36lpmu7prrx2@alap3.anarazel.de>
> I'm confused - that doesn't allow to inject flags to all in-core built
> files? So how does that fix your problem fully? Say
> e.g. llvmjit_inline.cpp won't get the flag, and thus not be
> reproducible?

The original itch being scratched here is extensions written in C++,
which need -ffile-prefix-map (or -fdebug-prefix-map) injected into
CXXFLAGS on top of the flags encoded in pgxs.mk. I originally picked
COPT because that works for injecting flags into pgxs.mk as well, but
PG_*FLAGS is better.

For the server build itself, it shouldn't be necessary to inject flags
because they are passed directly to ./configure. Admittedly I haven't
looked at reproducibility of PG11 yet as clang is still missing the
prefix-map feature, but https://reviews.llvm.org/D49466 is making good
progress.

Do we still want some CXXOPT flag for the server build? I can write a
patch, but someone else would need to do the bikeshedding how to name
it, and which of the existing knobs would set CXXFLAGS along. I don't
think I need that feature.

Christoph
-- 
Senior Berater, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Wed, Jan 30, 2019 at 02:41:01PM +0100, Christoph Berg wrote:
> Do we still want some CXXOPT flag for the server build? I can write a
> patch, but someone else would need to do the bikeshedding how to name
> it, and which of the existing knobs would set CXXFLAGS along. I don't
> think I need that feature.

If we don't directly need it, let's not add it now but let's revisit
the need if it proves necessary.  I propose to just commit the last
patch you sent, and back-patch to ease integration with existing
extensions.  Any objections?
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jan 30, 2019 at 02:41:01PM +0100, Christoph Berg wrote:
>> Do we still want some CXXOPT flag for the server build? I can write a
>> patch, but someone else would need to do the bikeshedding how to name
>> it, and which of the existing knobs would set CXXFLAGS along. I don't
>> think I need that feature.

> If we don't directly need it, let's not add it now but let's revisit
> the need if it proves necessary.

+1

> I propose to just commit the last
> patch you sent, and back-patch to ease integration with existing
> extensions.  Any objections?

A thought here:

 ifdef PG_CPPFLAGS
 override CPPFLAGS := $(PG_CPPFLAGS) $(CPPFLAGS)
 endif
+ifdef PG_CFLAGS
+override CFLAGS := $(PG_CFLAGS) $(CFLAGS)
+endif
+ifdef PG_CXXFLAGS
+override CXXFLAGS := $(PG_CXXFLAGS) $(CXXFLAGS)
+endif
+ifdef PG_LDFLAGS
+override LDFLAGS := $(PG_LDFLAGS) $(LDFLAGS)
+endif

This looks a bit copy-and-paste-y to me, in particular no thought
has been taken for the order of flags.  We found in configure that
it's better to add user-specified CFLAGS at the *end*, even though
injecting user-specified CPPFLAGS at the beginning is the right
thing.  This is because in, eg, "-O2 -O0" the last flag wins.
Presumably the same goes for CXXFLAGS.  I think it's right to
put user LDFLAGS first, though.  (The argument for CPPFLAGS and
LDFLAGS is that you want the user's -I and -L flags to go first.)

            regards, tom lane


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Wed, Jan 30, 2019 at 08:18:31PM -0500, Tom Lane wrote:
> This looks a bit copy-and-paste-y to me, in particular no thought
> has been taken for the order of flags.  We found in configure that
> it's better to add user-specified CFLAGS at the *end*, even though
> injecting user-specified CPPFLAGS at the beginning is the right
> thing.  This is because in, eg, "-O2 -O0" the last flag wins.
> Presumably the same goes for CXXFLAGS.  I think it's right to
> put user LDFLAGS first, though.  (The argument for CPPFLAGS and
> LDFLAGS is that you want the user's -I and -L flags to go first.)

Ah yes, good point about CFLAGS and LDFLAGS.  It would be better to
add a comment about that and document the difference, aka "prepend" or
"append" the flag values.

CXXFLAGS applies to compiler options like -g -O2 which you would like
to enforce for the C++ compiler, so it seems to me that like CFLAGS
the custom values should be added at the end and not at the beginning,
no?
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Thu, Jan 31, 2019 at 11:19:11AM +0900, Michael Paquier wrote:
> Ah yes, good point about CFLAGS and LDFLAGS.  It would be better to
> add a comment about that and document the difference, aka "prepend" or
> "append" the flag values.
>
> CXXFLAGS applies to compiler options like -g -O2 which you would like
> to enforce for the C++ compiler, so it seems to me that like CFLAGS
> the custom values should be added at the end and not at the beginning,
> no?

Attached is a patch doing that.  Thoughts?
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> Attached is a patch doing that.  Thoughts?

WFM.

            regards, tom lane


Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Michael Paquier
Date:
On Thu, Jan 31, 2019 at 10:51:11PM -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> Attached is a patch doing that.  Thoughts?
>
> WFM.

Thanks, pushed.
--
Michael

Attachment

Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well

From
Christoph Berg
Date:
Re: Michael Paquier 2019-02-03 <20190203090737.GA18892@paquier.xyz>
> >> Attached is a patch doing that.  Thoughts?
> > 
> > WFM.
> 
> Thanks, pushed.

Thanks. It makes much more sense that way round.

Christoph
-- 
Senior Berater, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz