Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
Re: Michael Paquier 2019-02-03 <20190203090737.ga18...@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
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
On Thu, Jan 31, 2019 at 10:51:11PM -0500, Tom Lane wrote: > Michael Paquier writes: >> Attached is a patch doing that. Thoughts? > > WFM. Thanks, pushed. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
Michael Paquier writes: > Attached is a patch doing that. Thoughts? WFM. regards, tom lane
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
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 diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index a6b77c1cfe..b5e59d542a 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -1353,7 +1353,34 @@ include $(PGXS) PG_CPPFLAGS -will be added to CPPFLAGS +will be prepended to CPPFLAGS + + + + + + PG_CFLAGS + + +will be appended to CFLAGS + + + + + + PG_CXXFLAGS + + +will be appended to CXXFLAGS + + + + + + PG_LDFLAGS + + +will be prepended to LDFLAGS diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk index d214cb9cf2..909a49f5be 100644 --- a/src/makefiles/pgxs.mk +++ b/src/makefiles/pgxs.mk @@ -52,7 +52,10 @@ # NO_INSTALLCHECK -- don't define an installcheck target, useful e.g. if # tests require special configuration, or don't use pg_regress # EXTRA_CLEAN -- extra files to remove in 'make clean' -# PG_CPPFLAGS -- will be added to CPPFLAGS +# PG_CPPFLAGS -- will be prepended to CPPFLAGS +# PG_CFLAGS -- will be appended to CFLAGS +# PG_CXXFLAGS -- will be appended to CXXFLAGS +# PG_LDFLAGS -- will be prepended to LDFLAGS # PG_LIBS -- will be added to PROGRAM link line # PG_LIBS_INTERNAL -- same, for references to libraries within build tree # SHLIB_LINK -- will be added to MODULE_big link line @@ -119,6 +122,15 @@ endif ifdef PG_CPPFLAGS override CPPFLAGS := $(PG_CPPFLAGS) $(CPPFLAGS) endif +ifdef PG_CFLAGS +override CFLAGS := $(CFLAGS) $(PG_CFLAGS) +endif +ifdef PG_CXXFLAGS +override CXXFLAGS := $(CXXFLAGS) $(PG_CXXFLAGS) +endif +ifdef PG_LDFLAGS +override LDFLAGS := $(PG_LDFLAGS) $(LDFLAGS) +endif # logic for HEADERS_* stuff signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
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 signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
Michael Paquier 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
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 signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
Re: Andres Freund 2019-01-30 <20190130015127.hciz36lpmu7pr...@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
Hi, On 2019-01-29 16:18:46 +0100, Christoph Berg wrote: > Re: Michael Paquier 2019-01-23 <20190123004722.ge3...@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
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 signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
Re: Michael Paquier 2019-01-23 <20190123004722.ge3...@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 >From 04bc55089a46697fec721aca6225a7ca1db6c66f Mon Sep 17 00:00:00 2001 From: Christoph Berg Date: Tue, 13 Nov 2018 11:35:26 +0100 Subject: [PATCH] Add PG_CFLAGS, PG_CXXFLAGS, and PG_LDFLAGS variables Add PG_CFLAGS, PG_CXXFLAGS, and PG_LDFLAGS variables to pgxs.mk which will append to the corresponding make variables. Notably, there was previously no way to pass custom CXXFLAGS to 3rd party extension module builds. (COPT and PROFILE support only CFLAGS and LDFLAGS) --- doc/src/sgml/extend.sgml | 27 +++ src/makefiles/pgxs.mk| 12 2 files changed, 39 insertions(+) diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index a6b77c1cfe..a94b216e77 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -1358,6 +1358,33 @@ include $(PGXS) + + PG_CFLAGS + + +will be added to CFLAGS + + + + + + PG_CXXFLAGS + + +will be added to CXXFLAGS + + + + + + PG_LDFLAGS + + +will be added to LDFLAGS + + + + PG_LIBS diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk index d214cb9cf2..56a8b26183 100644 --- a/src/makefiles/pgxs.mk +++ b/src/makefiles/pgxs.mk @@ -53,6 +53,9 @@ # tests require special configuration, or don't use pg_regress # EXTRA_CLEAN -- extra files to remove in 'make clean' # PG_CPPFLAGS -- will be added to CPPFLAGS +# PG_CFLAGS -- will be added to CFLAGS +# PG_CXXFLAGS -- will be added to CXXFLAGS +# PG_LDFLAGS -- will be added to LDFLAGS # PG_LIBS -- will be added to PROGRAM link line # PG_LIBS_INTERNAL -- same, for references to libraries within build tree # SHLIB_LINK -- will be added to MODULE_big link line @@ -119,6 +122,15 @@ endif 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 # logic for HEADERS_* stuff -- 2.20.1
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
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 signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
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
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
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
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 signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
On Tue, Jan 08, 2019 at 09:09:56AM +0100, Christoph Berg wrote: > Re: Andres Freund 2019-01-08 > <20190108011837.n4mx7dadvojv2...@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 signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
Re: Andres Freund 2019-01-08 <20190108011837.n4mx7dadvojv2...@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
Hi, On 2019-01-07 10:32:20 +0100, Christoph Berg wrote: > Re: To Michael Paquier 2019-01-07 <20190107091734.ga1...@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
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 signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
Re: To Michael Paquier 2019-01-07 <20190107091734.ga1...@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 >From 5abf37ae89e1680359e899de46e7ed709fc37125 Mon Sep 17 00:00:00 2001 From: Christoph Berg Date: Tue, 13 Nov 2018 11:35:26 +0100 Subject: [PATCH] Add more flexibility for passing custom compiler flags The existing machinery for extending CFLAGS and LDFLAGS via COPT and PROFILE neglected to extend CXXFLAGS as well, causing third party extensions written in C++ not to get the extra flags. Also add PG_CFLAGS, PG_CXXFLAGS, and PG_LDFLAGS variables to pgxs.mk which will append to the corresponding make variables. --- doc/src/sgml/installation.sgml | 2 +- src/Makefile.global.in | 2 ++ src/makefiles/pgxs.mk | 12 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml index 3c9544cc27..98af5d6f56 100644 --- a/doc/src/sgml/installation.sgml +++ b/doc/src/sgml/installation.sgml @@ -1536,7 +1536,7 @@ su - postgres it will break many of configure's built-in tests. To add such flags, include them in the COPT environment variable while running make. The contents of COPT - are added to both the CFLAGS and LDFLAGS + are added to both the CFLAGS, CXXFLAGS, and LDFLAGS options set up by configure. For example, you could do make COPT='-Werror' diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 41c131412e..665923d9c3 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -676,11 +676,13 @@ endif # ifdef COPT CFLAGS += $(COPT) + CXXFLAGS += $(COPT) LDFLAGS += $(COPT) endif ifdef PROFILE CFLAGS += $(PROFILE) + CXXFLAGS += $(PROFILE) LDFLAGS += $(PROFILE) endif diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk index d214cb9cf2..56a8b26183 100644 --- a/src/makefiles/pgxs.mk +++ b/src/makefiles/pgxs.mk @@ -53,6 +53,9 @@ # tests require special configuration, or don't use pg_regress # EXTRA_CLEAN -- extra files to remove in 'make clean' # PG_CPPFLAGS -- will be added to CPPFLAGS +# PG_CFLAGS -- will be added to CFLAGS +# PG_CXXFLAGS -- will be added to CXXFLAGS +# PG_LDFLAGS -- will be added to LDFLAGS # PG_LIBS -- will be added to PROGRAM link line # PG_LIBS_INTERNAL -- same, for references to libraries within build tree # SHLIB_LINK -- will be added to MODULE_big link line @@ -119,6 +122,15 @@ endif 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 # logic for HEADERS_* stuff -- 2.19.2
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
Re: Michael Paquier 2019-01-04 <20190104133305.gg2...@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 >From 51226dc97039b42e7fc8670a895ec51b8d715b12 Mon Sep 17 00:00:00 2001 From: Christoph Berg Date: Tue, 13 Nov 2018 11:35:26 +0100 Subject: [PATCH] Add more flexibility for passing custom compiler flags The existing machinery for extending CFLAGS and LDFLAGS via COPT and PROFILE neglected to extend CXXFLAGS as well, causing third party extensions written in C++ not to get the extra flags. Also add PG_CFLAGS, PG_CXXFLAGS, and PG_LDFLAGS variables to pgxs.mk which will append to the corresponding make variables. --- src/Makefile.global.in | 6 ++ src/makefiles/pgxs.mk | 12 2 files changed, 18 insertions(+) diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 41c131412e..8ac511756c 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -679,8 +679,14 @@ ifdef COPT LDFLAGS += $(COPT) endif +ifdef CXXOPT + CXXFLAGS += $(CXXOPT) + LDFLAGS += $(CXXOPT) +endif + ifdef PROFILE CFLAGS += $(PROFILE) + CXXFLAGS += $(PROFILE) LDFLAGS += $(PROFILE) endif diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk index d214cb9cf2..56a8b26183 100644 --- a/src/makefiles/pgxs.mk +++ b/src/makefiles/pgxs.mk @@ -53,6 +53,9 @@ # tests require special configuration, or don't use pg_regress # EXTRA_CLEAN -- extra files to remove in 'make clean' # PG_CPPFLAGS -- will be added to CPPFLAGS +# PG_CFLAGS -- will be added to CFLAGS +# PG_CXXFLAGS -- will be added to CXXFLAGS +# PG_LDFLAGS -- will be added to LDFLAGS # PG_LIBS -- will be added to PROGRAM link line # PG_LIBS_INTERNAL -- same, for references to libraries within build tree # SHLIB_LINK -- will be added to MODULE_big link line @@ -119,6 +122,15 @@ endif 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 # logic for HEADERS_* stuff -- 2.19.2
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
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 signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
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
(Sorry for the delayed response here.)
Re: Andres Freund 2018-11-13 <20181113223330.2ql7tg33hhh6h...@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
Hi, On 2018-11-13 17:27:17 -0500, Tom Lane wrote: > Andres Freund 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
Andres Freund 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
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
