Patchwork multipath-tools: Makefile: Respect standard toolchain related envvars

login
register
mail settings
Submitter Thomas Deutschmann
Date Nov. 29, 2017, 11:23 p.m.
Message ID <20171129232311.95483-1-whissi@gentoo.org>
Download mbox | patch
Permalink /patch/10084465/
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show

Comments

Thomas Deutschmann - Nov. 29, 2017, 11:23 p.m.
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(-)
Bart Van Assche - Dec. 12, 2017, 3:28 p.m.
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
Thomas Deutschmann - Dec. 12, 2017, 4:09 p.m.
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.
Bart Van Assche - Dec. 12, 2017, 4:30 p.m.
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
Xose Vazquez Perez - Dec. 13, 2017, 1:28 p.m.
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
Martin Wilck - Dec. 18, 2017, 2:40 p.m.
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
Martin Wilck - Jan. 11, 2018, 10:24 p.m.
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.
Thomas Deutschmann - Jan. 11, 2018, 11:26 p.m.
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.
Martin Wilck - Jan. 12, 2018, 10:02 a.m.
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
Thomas Deutschmann - Jan. 12, 2018, 3:46 p.m.
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...
Martin Wilck - Jan. 12, 2018, 10:27 p.m.
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

Patch

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.