| Message ID | 20171129232311.95483-1-whissi@gentoo.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On Thu, 2017-11-30 at 00:23 +0100, Thomas Deutschmann wrote: > The Makefile overrides standard envvars that control the toolchain flags. > This patch should set things right without reducing default behavior. > > Signed-off-by: Thomas Deutschmann <whissi@gentoo.org> > --- > Makefile.inc | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Makefile.inc b/Makefile.inc > index 29c290a2..951d58fc 100644 > --- a/Makefile.inc > +++ b/Makefile.inc > @@ -90,11 +90,12 @@ OPTFLAGS = -O2 -g -pipe -Wall -Wextra -Wformat=2 -Werror=implicit-int \ > -Wp,-D_FORTIFY_SOURCE=2 $(STACKPROT) \ > --param=ssp-buffer-size=4 > > -CFLAGS = $(OPTFLAGS) -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\" > +CFLAGS ?= $(OPTFLAGS) > +CFLAGS += -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\" > BIN_CFLAGS = -fPIE -DPIE > LIB_CFLAGS = -fPIC > SHARED_FLAGS = -shared > -LDFLAGS = -Wl,-z,relro -Wl,-z,now > +LDFLAGS += -Wl,-z,relro -Wl,-z,now > BIN_LDFLAGS = -pie > > # Check whether a function with name $1 has been declared in header file $2. Hello Thomas, I agree that we need a way to specify additional compilation flags. However, is OPTFLAGS really a standard? Aren't most developers used to set CFLAGS to specify additional compilation flags? Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi Bart, On 2017-12-12 16:28, Bart Van Assche wrote: > I agree that we need a way to specify additional compilation flags. However, is > OPTFLAGS really a standard? Aren't most developers used to set CFLAGS to specify > additional compilation flags? When I created the patch I searched in the source code of other projects after "OPTFLAGS" usage and came to the conclusion that this isn't a standard. However, my patch shouldn't break any existing use cases. So my understanding was that "OPTFLAGS" is used like the default set of flags in case the user hasn't set any flags on its own. However, the project may still need to add a special flag which should be set everywhere. That's why I created the patch the way I did: 1) User can provide own CFLAGS in which case the default set of flags (OPTFLAGS) aren't used. 2) We now extend set CFLAGS by adding our additional flags (-DRUN_DIR..) we want to be set everywhere. Your patch will do the same but require downstream to unset/negate default flags. I don't really care if we pick your patch or my patch.
On Tue, 2017-12-12 at 17:09 +0100, Thomas Deutschmann wrote: > On 2017-12-12 16:28, Bart Van Assche wrote: > > I agree that we need a way to specify additional compilation flags. However, is > > OPTFLAGS really a standard? Aren't most developers used to set CFLAGS to specify > > additional compilation flags? > > When I created the patch I searched in the source code of other projects > after "OPTFLAGS" usage and came to the conclusion that this isn't a > standard. > > However, my patch shouldn't break any existing use cases. > > So my understanding was that "OPTFLAGS" is used like the default set of > flags in case the user hasn't set any flags on its own. However, the > project may still need to add a special flag which should be set everywhere. > > That's why I created the patch the way I did: > > 1) User can provide own CFLAGS in which case the default set of flags > (OPTFLAGS) aren't used. > > 2) We now extend set CFLAGS by adding our additional flags (-DRUN_DIR..) > we want to be set everywhere. > > Your patch will do the same but require downstream to unset/negate > default flags. I don't really care if we pick your patch or my patch. Hello Thomas, I'm fine with dropping my patch and proceeding with your patch. However, I think that any CFLAGS specified on the make command line or in the environment should be appended at the end of the CFLAGS/OPTFLAGS specified by the Makefile. Otherwise it's not possible to override e.g. the -W... flags specified in the Makefile from the command line. Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 12/12/2017 04:28 PM, Bart Van Assche wrote: > On Thu, 2017-11-30 at 00:23 +0100, Thomas Deutschmann wrote: >> The Makefile overrides standard envvars that control the toolchain flags. >> This patch should set things right without reducing default behavior. >> >> Signed-off-by: Thomas Deutschmann <whissi@gentoo.org> >> --- >> Makefile.inc | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/Makefile.inc b/Makefile.inc >> index 29c290a2..951d58fc 100644 >> --- a/Makefile.inc >> +++ b/Makefile.inc >> @@ -90,11 +90,12 @@ OPTFLAGS = -O2 -g -pipe -Wall -Wextra -Wformat=2 -Werror=implicit-int \ >> -Wp,-D_FORTIFY_SOURCE=2 $(STACKPROT) \ >> --param=ssp-buffer-size=4 >> >> -CFLAGS = $(OPTFLAGS) -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\" >> +CFLAGS ?= $(OPTFLAGS) >> +CFLAGS += -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\" >> BIN_CFLAGS = -fPIE -DPIE >> LIB_CFLAGS = -fPIC >> SHARED_FLAGS = -shared >> -LDFLAGS = -Wl,-z,relro -Wl,-z,now >> +LDFLAGS += -Wl,-z,relro -Wl,-z,now >> BIN_LDFLAGS = -pie >> >> # Check whether a function with name $1 has been declared in header file $2. > > Hello Thomas, > > I agree that we need a way to specify additional compilation flags. However, is > OPTFLAGS really a standard? Aren't most developers used to set CFLAGS to specify > additional compilation flags? OPTFLAGS is used by RPM distributions(Fedora, openSUSE and derivatives). -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, 2017-12-13 at 14:28 +0100, Xose Vazquez Perez wrote: > On 12/12/2017 04:28 PM, Bart Van Assche wrote: > > On Thu, 2017-11-30 at 00:23 +0100, Thomas Deutschmann wrote: > > > The Makefile overrides standard envvars that control the > > > toolchain flags. > > > This patch should set things right without reducing default > > > behavior. > > > > > > Signed-off-by: Thomas Deutschmann <whissi@gentoo.org> > > > > > > > Hello Thomas, > > > > I agree that we need a way to specify additional compilation flags. > > However, is > > OPTFLAGS really a standard? Aren't most developers used to set > > CFLAGS to specify > > additional compilation flags? > > OPTFLAGS is used by RPM distributions(Fedora, openSUSE and > derivatives). I'm not sure what you mean. At openSUSE, we simply set OPTFLAGS=%{optflags} in the spec file. RPM_OPT_FLAGS aka %{optflags} is a standard for RPM-based distros, but "OPTFLAGS" is not. Martin
On Thu, 2017-11-30 at 00:23 +0100, Thomas Deutschmann wrote: > The Makefile overrides standard envvars that control the toolchain > flags. > This patch should set things right without reducing default behavior. I apologize for the very late reply. I'm not sure what you mean with "standard envvars" here. Is it "CFLAGS" and "LDFLAGS"? If that's what you mean, I disagree. CFLAGS handling is correct this way - CFLAGS should represent the final list of options passed to the compiler. Typically the Makefile assembles this list from internal project settings and user input. Currently, setting OPTFLAGS is the preferred way to customize multipath-tools build. If changes are needed, I'd prefer Bart's suggestion. Martin > > Signed-off-by: Thomas Deutschmann <whissi@gentoo.org> > --- > Makefile.inc | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Makefile.inc b/Makefile.inc > index 29c290a2..951d58fc 100644 > --- a/Makefile.inc > +++ b/Makefile.inc > @@ -90,11 +90,12 @@ OPTFLAGS = -O2 -g -pipe -Wall -Wextra > -Wformat=2 -Werror=implicit-int \ > -Wp,-D_FORTIFY_SOURCE=2 $(STACKPROT) \ > --param=ssp-buffer-size=4 > > -CFLAGS = $(OPTFLAGS) -DLIB_STRING=\"${LIB}\" > -DRUN_DIR=\"${RUN}\" > +CFLAGS ?= $(OPTFLAGS) > +CFLAGS += -DLIB_STRING=\"${LIB}\" > -DRUN_DIR=\"${RUN}\" > BIN_CFLAGS = -fPIE -DPIE > LIB_CFLAGS = -fPIC > SHARED_FLAGS = -shared > -LDFLAGS = -Wl,-z,relro -Wl,-z,now > +LDFLAGS += -Wl,-z,relro -Wl,-z,now > BIN_LDFLAGS = -pie > > # Check whether a function with name $1 has been declared in header > file $2.
On 2018-01-11 23:24, Martin Wilck wrote: > I'm not sure what you mean with "standard envvars" here. Is it "CFLAGS" > and "LDFLAGS"? If that's what you mean, I disagree. Yes, I was especially talking about CFLAGS. > CFLAGS handling is correct this way - CFLAGS should represent the final > list of options passed to the compiler. Typically the Makefile > assembles this list from internal project settings and user input. > > Currently, setting OPTFLAGS is the preferred way to customize > multipath-tools build. And this works for you with the current multipath-tools package? Sorry, maybe I am missing something but from my understanding the Makefile is currently setting OPTFLAGS via OPTFLAGS = -O2 -g .... So any OPTFLAGS value you are currently passing should be ignored at the moment. That's why Bart and I independently from each other created this patch. > If changes are needed, I'd prefer Bart's suggestion. Like said, Bart's variant works for me, too. We can pick his patch.
On Fri, 2018-01-12 at 00:26 +0100, Thomas Deutschmann wrote: > On 2018-01-11 23:24, Martin Wilck wrote: > > > > Currently, setting OPTFLAGS is the preferred way to customize > > multipath-tools build. > > And this works for you with the current multipath-tools package? > Sorry, > maybe I am missing something but from my understanding the Makefile > is > currently setting OPTFLAGS via > > OPTFLAGS = -O2 -g .... Yes it works. Your can do e.g. make OPTFLAGS="-g -O3 -fsanitize=address" > So any OPTFLAGS value you are currently passing should be ignored at > the > moment. That's why Bart and I independently from each other created > this > patch. OPTFLAGS from the environment is ignored, but OPTFLAGS from the command line is honored. https://www.gnu.org/software/make/manual/make.html#Environment https://www.gnu.org/software/make/manual/make.html#Overriding Regards, Martin
Hi,
On 2018-01-12 11:02, Martin Wilck wrote:
> make OPTFLAGS="-g -O3 -fsanitize=address"
Thanks, this works for me.
OK, I am now pulling my patch suggestion. No longer required.
Regarding Bart's patch: Still not sure how important things like
-D_FORTIFY_SOURCE=2 $(STACKPROT) --param=ssp-buffer-size=4
are. We (Gentoo) will now probably set
make OPTFLAGS="${CFLAGS}"
so we will lose this per default. Bart's patch would preserve these
defaults...
On Fri, 2018-01-12 at 16:46 +0100, Thomas Deutschmann wrote: > > Regarding Bart's patch: Still not sure how important things like > > -D_FORTIFY_SOURCE=2 $(STACKPROT) --param=ssp-buffer-size=4 > > are. Most of this came came from Red Hat: commit 1fce66911f4c15e3a9c09948f4c146d859bc1834 Author: Benjamin Marzinski <bmarzins@redhat.com> Date: Thu May 24 23:57:42 2012 -0500 multipath: Build with standard rpm cflags This patch makes multipath build with the standard redhat rpm cflags, which can help catch some code errors. I haven't questioned these so far. I don't think they have negative impact, and might help catching errors. Martin
diff --git a/Makefile.inc b/Makefile.inc index 29c290a2..951d58fc 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -90,11 +90,12 @@ OPTFLAGS = -O2 -g -pipe -Wall -Wextra -Wformat=2 -Werror=implicit-int \ -Wp,-D_FORTIFY_SOURCE=2 $(STACKPROT) \ --param=ssp-buffer-size=4 -CFLAGS = $(OPTFLAGS) -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\" +CFLAGS ?= $(OPTFLAGS) +CFLAGS += -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\" BIN_CFLAGS = -fPIE -DPIE LIB_CFLAGS = -fPIC SHARED_FLAGS = -shared -LDFLAGS = -Wl,-z,relro -Wl,-z,now +LDFLAGS += -Wl,-z,relro -Wl,-z,now BIN_LDFLAGS = -pie # Check whether a function with name $1 has been declared in header file $2.
The Makefile overrides standard envvars that control the toolchain flags. This patch should set things right without reducing default behavior. Signed-off-by: Thomas Deutschmann <whissi@gentoo.org> --- Makefile.inc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)