Bug 10877 - Flags from enviroment are put before Samba's own
Flags from enviroment are put before Samba's own
Status: ASSIGNED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Build
4.1.12
All All
: P5 normal
: ---
Assigned To: Ralph Böhme
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-14 18:08 UTC by Andrea D'Amore
Modified: 2016-03-02 07:58 UTC (History)
5 users (show)

See Also:


Attachments
build log excerpt (3.56 KB, text/plain)
2014-10-14 18:08 UTC, Andrea D'Amore
no flags Details
Manual compilation with CPPFLAGS from env at the end (2.21 KB, text/plain)
2014-10-15 17:45 UTC, Ralph Böhme
no flags Details
Put CPPFLAGS behind internal INCLUDES (1.02 KB, patch)
2014-10-16 08:42 UTC, Ralph Böhme
no flags Details
testcase as automake project (89.22 KB, application/octet-stream)
2014-10-17 10:22 UTC, Ralph Böhme
no flags Details
Patch for master (2.30 KB, patch)
2014-12-21 16:41 UTC, Ralph Böhme
no flags Details
Patch for master (2.30 KB, patch)
2014-12-21 16:41 UTC, Ralph Böhme
no flags Details
WIP patch for search path ordering (3.09 KB, patch)
2015-04-26 14:08 UTC, Ralph Böhme
no flags Details
Patch for master (3.04 KB, patch)
2015-05-22 09:25 UTC, Ralph Böhme
no flags Details
Patch for 4.2 (5.60 KB, patch)
2015-05-22 09:25 UTC, Ralph Böhme
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrea D'Amore 2014-10-14 18:08:29 UTC
Created attachment 10348 [details]
build log excerpt

The build system is putting flags from environment before own flags.
This leads in an error on Mac OS X when building with MacPorts that is providing a own version of gssapi.h header.

I'm attaching a relevant excerpt of the build log.

This ticket should be assigned to Ralph Böhme <slow@samba.org>.
Comment 1 Ralph Böhme 2014-10-15 17:43:47 UTC
Indeed, manually fixing the compilation command by putting -I/opt/include at the end works, see attachment.
Comment 2 Ralph Böhme 2014-10-15 17:45:10 UTC
Created attachment 10349 [details]
Manual compilation with CPPFLAGS from env at the end
Comment 3 Ralph Böhme 2014-10-15 17:48:45 UTC
This is what automake can teach us on the subject of user flags versus project flags:
<http://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html>

"Automake always uses two of these variables when compiling C sources files. When compiling an object file for the mumble target, the first variable will be mumble_CPPFLAGS if it is defined, or AM_CPPFLAGS otherwise. The second variable is always CPPFLAGS."
Comment 4 Ralph Böhme 2014-10-16 08:42:38 UTC
Created attachment 10350 [details]
Put CPPFLAGS behind internal INCLUDES

Urks, this is probably worse then anticipated.

The canonical environment variable for include paths is CPPFLAGS and this is where Macports correctly inserts it's path (CPPFLAGS=-I/opt/local/include).

Otoh, waf when building the list of include path at configure time from config checks puts it's includes in a different variables (afaict interenally variables suffixed by _CCINCFLAGS), which is just fine.

At some point, when driving the compiler, all needed flags are put together in a specific order, which is defined, afaict, in buildtools/wafadmin/Tools/cc.py:

cc_str = '${CC} ${CCFLAGS} ${CPPFLAGS} ${_CCINCFLAGS} ${_CCDEFFLAGS} ${CC_SRC_F}${SRC} ${CC_TGT_F}${TGT}'

CPPFLAGS is put before _CCINCFLAGS, which is probably not correct. What should be used instead is

cc_str = '${CC} ${CCFLAGS} ${_CCINCFLAGS} ${CPPFLAGS} ${_CCDEFFLAGS} ${CC_SRC_F}${SRC} ${CC_TGT_F}${TGT}'

Attached patch for 4.1.2 fixes this and makes the build pass in Macports. But this can be applied to Samba in general as it's chaning an upsteam waf file, we'd need a more isolated patch for buildtools/wafsamba/ that changes the upstream default.

I think this needs discussion on samba-technical and waf-users. I may be completely wrong with my analysis of the situation, hell, I can't even program python! :)
Comment 5 Andrea D'Amore 2014-10-16 15:06:24 UTC
I confirm being able to build 4.1.12 on 10.9.5 with MacPorts by patching buildtools/ as suggested.
Comment 6 Ralph Böhme 2014-10-17 09:41:16 UTC
Here's a testcase that demonstrates the issue. Expected ouput is

  foo: FOO

in either case, but due do the wrong ordering if CPPFLAGS is defined the actual output is

  foo: BAR

[ralph@fedora20 waftests]$ tree
.
├── external_include
│   └── myheader.h
├── internal_include
│   └── myheader.h
├── main.c
└── wscript

2 directories, 4 files
[ralph@fedora20 waftests]$ find . -type f | xargs -t -n 1 cat
cat ./main.c 
#include <stdio.h>
#include "myheader.h"

int main(int argc, char **argv) {
    printf("foo: %s\n", FOO);
    return 0;
}
cat ./external_include/myheader.h 
#ifndef MYHEADER_H
#define MYHEADER_H
#define FOO "BAR"
#endif
cat ./internal_include/myheader.h 
#ifndef MYHEADER_H
#define MYHEADER_H
#define FOO "FOO"
#endif
cat ./wscript 
#!/usr/bin/env python

top = '.'
out = 'build'

def options(opt):
    opt.load('compiler_c')

def configure(conf):
    conf.load('compiler_c')

def build(bld):
    bld.program(source='main.c',target='app',includes = 'internal_include')
[ralph@fedora20 waftests]$ waf distclean && waf configure build -v && ./build/app 
'distclean' finished successfully (0.000s)
Setting top to                           : /home/ralph/waftests 
Setting out to                           : /home/ralph/waftests/build 
Checking for 'c_bgxlc' (c compiler)      : not found 
Checking for 'gcc' (c compiler)          : /usr/bin/gcc 
'configure' finished successfully (0.047s)
Waf: Entering directory `/home/ralph/waftests/build'
[1/2] c: main.c -> build/main.c.1.o
11:38:12 runner ['/usr/bin/gcc', '-I/home/ralph/waftests/build/internal_include', '-I/home/ralph/waftests/internal_include', '../main.c', '-c', '-o', 'main.c.1.o']
[2/2] cprogram: build/main.c.1.o -> build/app
11:38:12 runner ['/usr/bin/gcc', 'main.c.1.o', '-o', '/home/ralph/waftests/build/app', '-Wl,-Bstatic', '-Wl,-Bdynamic']
Waf: Leaving directory `/home/ralph/waftests/build'
'build' finished successfully (0.073s)
foo: FOO
[ralph@fedora20 waftests]$ waf distclean && CPPFLAGS=-I$PWD/external_include waf configure build -v && ./build/app 
'distclean' finished successfully (0.001s)
Setting top to                           : /home/ralph/waftests 
Setting out to                           : /home/ralph/waftests/build 
Checking for 'c_bgxlc' (c compiler)      : not found 
Checking for 'gcc' (c compiler)          : /usr/bin/gcc 
'configure' finished successfully (0.048s)
Waf: Entering directory `/home/ralph/waftests/build'
[1/2] c: main.c -> build/main.c.1.o
11:38:26 runner ['/usr/bin/gcc', '-I/home/ralph/waftests/external_include', '-I/home/ralph/waftests/build/internal_include', '-I/home/ralph/waftests/internal_include', '../main.c', '-c', '-o', 'main.c.1.o']
[2/2] cprogram: build/main.c.1.o -> build/app
11:38:26 runner ['/usr/bin/gcc', 'main.c.1.o', '-o', '/home/ralph/waftests/build/app', '-Wl,-Bstatic', '-Wl,-Bdynamic']
Waf: Leaving directory `/home/ralph/waftests/build'
'build' finished successfully (0.072s)
foo: BAR
[ralph@fedora20 waftests]$
Comment 7 Ralph Böhme 2014-10-17 10:22:21 UTC
Created attachment 10354 [details]
testcase as automake project

Same testcase with automake:

[ralph@fedora20 automaketest]$ make clean && make && ./app
test -z "app" || rm -f app
rm -f *.o
gcc -DPACKAGE_NAME=\"app\" -DPACKAGE_TARNAME=\"app\" -DPACKAGE_VERSION=\"1.0.0\" -DPACKAGE_STRING=\"app\ 1.0.0\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"app\" -DVERSION=\"1.0.0\" -I.  -I./internal_include/   -g -O2 -MT app-main.o -MD -MP -MF .deps/app-main.Tpo -c -o app-main.o `test -f 'main.c' || echo './'`main.c
mv -f .deps/app-main.Tpo .deps/app-main.Po
gcc  -g -O2   -o app app-main.o  
foo: FOO
[ralph@fedora20 automaketest]$ make clean && CPPFLAGS=$PWD/external_include make && ./app
test -z "app" || rm -f app
rm -f *.o
gcc -DPACKAGE_NAME=\"app\" -DPACKAGE_TARNAME=\"app\" -DPACKAGE_VERSION=\"1.0.0\" -DPACKAGE_STRING=\"app\ 1.0.0\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"app\" -DVERSION=\"1.0.0\" -I.  -I./internal_include/   -g -O2 -MT app-main.o -MD -MP -MF .deps/app-main.Tpo -c -o app-main.o `test -f 'main.c' || echo './'`main.c
mv -f .deps/app-main.Tpo .deps/app-main.Po
gcc  -g -O2   -o app app-main.o  
foo: FOO
Comment 8 Ralph Böhme 2014-12-21 16:41:56 UTC
Created attachment 10553 [details]
Patch for master

Patch for master pair-programmed with metze that ensures CPPFLAGS and LDFLAGS are always appended at the end of the compiler/linker invocation.
Comment 9 Ralph Böhme 2014-12-21 16:41:58 UTC
Created attachment 10554 [details]
Patch for master

Patch for master pair-programmed with metze that ensures CPPFLAGS and LDFLAGS are always appended at the end of the compiler/linker invocation.
Comment 10 Albert Chin 2015-02-19 06:15:31 UTC
Adding LDFLAGS to the end is a problem. It doesn't make sense to add -L after -l. With this patch, if a user tells Samba to search for custom libs in some path /foo because they want to override the default system library, then it won't work because the -L appears at the end with this patch. Note the same applies to -I, depending on the compiler. Consider the following:

$ cat z.c
#include <zlib.h>

$ cc -V
cc: Sun C 5.12 SunOS_sparc Patch 148917-06 2013/06/11
$ cc -E z.c | grep zlib.h
# 1 "/usr/include/zlib.h"
# 35 "/usr/include/zlib.h"
$ cc -I/opt/fsw/libz12/include -E z.c | grep zlib.h
# 1 "/opt/fsw/libz12/include/zlib.h"
# 35 "/opt/fsw/libz12/include/zlib.h"
$ cc -I/usr/include -I/opt/fsw/libz12/include -E z.c | grep zlib.h
# 1 "/usr/include/zlib.h"
# 35 "/usr/include/zlib.h"

Note that with the Sun C compiler, the path to zlib.h depends on which -I appears first. Now, let's look at gcc-4.4.6:

$ gcc -E z.c | grep zlib.h
# 1 "/usr/include/zlib.h" 1 3 4
...
$ gcc -I/opt/fsw/libz12/include -E z.c | grep zlib.h
# 1 "/opt/fsw/libz12/include/zlib.h" 1
...
$ gcc -I/usr/include -I/opt/fsw/libz12/include -E z.c | grep zlib.h
# 1 "/opt/fsw/libz12/include/zlib.h" 1
...

So, with GCC, the -I path that appears last "wins". With the Sun C compiler, the -I path that appears first "wins". So, while this patch "fixes" things with GCC, it breaks compilation on Solaris.
Comment 11 Jelmer Vernooij 2015-04-25 13:48:24 UTC
Adding LDFLAGS to the end is indeed a problem. Unfortunately this breaks the use of -Wl,--as-needed as used in Debian. -Wl,--as-needed is applied to any libraries specified *after* it on the command-line.
Comment 12 Ralph Böhme 2015-04-26 13:51:10 UTC
(In reply to Albert Chin from comment #10)
> Adding LDFLAGS to the end is a problem. It doesn't make sense to add -L
> after -l.
You and Jelmer are absolutely right! This was not intended and I'm sorry that this escaped me. The intention was to put the user env LDFLAGS behind our internal -L search paths, but not behind LIBS.

> So, with GCC, the -I path that appears last "wins". With the Sun C
> compiler, the -I path that appears first "wins". So, while this patch
> "fixes" things with GCC, it breaks compilation on Solaris.

I guess then we're screwed, not sure what autotools does wrt this. Do they adapt the commandline order depending on the compiler? I don't think so.

The problem this patch was trying to address, is that the paths the user env LDFLAGS -L and CPPFLAGS -I point at may contain Samba headers and libraries that may get picked up instead of the in-tree versions. This is wrong. We have to ensure our build correctly picks up in-tree versions of libraries.

I'm working on an correct fix addressing this, as long we should indeed revert my broken commit. Sorry for the hassle!
Comment 13 Ralph Böhme 2015-04-26 14:08:49 UTC
Created attachment 10991 [details]
WIP patch for search path ordering

Can you review and/or test this version. Thanks!
Comment 14 Ralph Böhme 2015-05-22 09:25:20 UTC
Created attachment 11083 [details]
Patch for master
Comment 15 Ralph Böhme 2015-05-22 09:25:59 UTC
Created attachment 11084 [details]
Patch for 4.2
Comment 16 Ralph Böhme 2016-03-02 07:58:45 UTC
Parsing user (C|LD)FLAGS into distinct variables for library search paths (-I|L) and compiler/linker flags (like --as-needed) may work.