cmd/go: add SWIGFLAGS environment variable to propagate '-D' flags

[ghost]

This issue is seen in go1.7.1 on both darwin/amd64 and linux/386. It probably exists on other platforms.

Consider the following simple program.

bash-3.2$ ls -l
total 24
-rw-r--r-- 1 hormozzarnani staff 51 Dec 15 12:58 bug_D_prop.i
lrwxr-xr-x 1 hormozzarnani staff 12 Dec 15 12:56 bug_D_prop.swigcxx -> bug_D_prop.i
-rw-r--r-- 1 hormozzarnani staff 70 Dec 15 12:56 package.go
bash-3.2$ for F in *.go *.i; do echo "==> $F <=="; cat -n $F; done
==> package.go <==
1 package bug_D_prop
2
3 // #cgo CPPFLAGS: -I/foo/bar/bam -DDIE
4 import "C"
==> bug_D_prop.i <==
1 %module bug_D_prop
2 #ifdef DIE
3 %sytanx_error
4 #endif
bash-3.2$

Running "go build" in this directory should fail because line 3, which has a syntax error, should be conditionally included, as package.go defines the symbol DIE. The error should be the same as that emitted when running swig manually with the intended preprocessor:

bash-3.2$ swig -go -cgo -intgosize 64 -c++ -DDIE bug_D_prop.i
bug_D_prop.i:5: Error: Syntax error in input(1).
bash-3.2$

However, running "go build" will only propagate the -I flag and not the -D flag from CPPFLAGS to swig:

bash-3.2$ go build -x
WORK=/var/folders/m6/8y6rbg4s2td16l0z7f1pm1_c0000gn/T/go-build554145057
mkdir -p $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/
mkdir -p $WORK/swig-sandbox/bug-reports/go-swig/
swig -version
cd $WORK
/usr/local/go/pkg/tool/darwin_amd64/compile -o ./swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/go.o -trimpath . -p main -complete -buildid 0bc81afb4bf75f8e57bb3ceab2b5427ab06b67e9 -D _$WORK ./swig_intsize.go
cd /Users/hormozzarnani/go/src/swig-sandbox/bug-reports/go-swig/D-flag-propagation
swig -go -cgo -intgosize 64 -module bug_D_prop -o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/bug_D_prop_wrap.cxx -outdir $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/ -I/foo/bar/bam -c++ bug_D_prop.swigcxx
CGO_LDFLAGS="-g" "-O2" /usr/local/go/pkg/tool/darwin_amd64/cgo -objdir $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/ -importpath swig-sandbox/bug-reports/go-swig/D-flag-propagation -- -I/foo/bar/bam -DDIE -I $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/ package.go $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/bug_D_prop.go
cd $WORK
clang -fdebug-prefix-map=a=b -c trivial.c
clang -gno-record-gcc-switches -c trivial.c
cd /Users/hormozzarnani/go/src/swig-sandbox/bug-reports/go-swig/D-flag-propagation
clang -I . -fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=$WORK=/tmp/go-build -gno-record-gcc-switches -fno-common -I/foo/bar/bam -DDIE -I $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/ -g -O2 -o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_main.o -c $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_main.c
clang -I . -fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=$WORK=/tmp/go-build -gno-record-gcc-switches -fno-common -I/foo/bar/bam -DDIE -I $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/ -g -O2 -o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_export.o -c $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_export.c
clang -I . -fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=$WORK=/tmp/go-build -gno-record-gcc-switches -fno-common -I/foo/bar/bam -DDIE -I $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/ -g -O2 -o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/package.cgo2.o -c $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/package.cgo2.c
clang -I . -fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=$WORK=/tmp/go-build -gno-record-gcc-switches -fno-common -I/foo/bar/bam -DDIE -I $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/ -g -O2 -o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_var_folders_m6_8y6rbg4s2td16l0z7f1pm1_c0000gn_T_go-build554145057_swig-sandbox_bug-reports_go-swig_D-flag-propagation__obj_bug_D_prop.cgo2.o -c $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_var_folders_m6_8y6rbg4s2td16l0z7f1pm1_c0000gn_T_go-build554145057_swig-sandbox_bug-reports_go-swig_D-flag-propagation__obj_bug_D_prop.cgo2.c
clang++ -I . -fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=$WORK=/tmp/go-build -gno-record-gcc-switches -fno-common -I/foo/bar/bam -DDIE -I $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/ -g -O2 -o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_var_folders_m6_8y6rbg4s2td16l0z7f1pm1_c0000gn_T_go-build554145057_swig-sandbox_bug-reports_go-swig_D-flag-propagation__obj_bug_D_prop_wrap.cxx.o -c $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/bug_D_prop_wrap.cxx
clang++ -I . -fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=$WORK=/tmp/go-build -gno-record-gcc-switches -fno-common -o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/cgo.o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_main.o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_export.o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/package.cgo2.o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_var_folders_m6_8y6rbg4s2td16l0z7f1pm1_c0000gn_T_go-build554145057_swig-sandbox_bug-reports_go-swig_D-flag-propagation__obj_bug_D_prop.cgo2.o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_var_folders_m6_8y6rbg4s2td16l0z7f1pm1_c0000gn_T_go-build554145057_swig-sandbox_bug-reports_go-swig_D-flag-propagation__obj_bug_D_prop_wrap.cxx.o -g -O2
/usr/local/go/pkg/tool/darwin_amd64/cgo -objdir $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/ -dynpackage bug_D_prop -dynimport $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/cgo.o -dynout $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_import.go
cd $WORK
clang -no-pie -c trivial.c
cd /Users/hormozzarnani/go/src/swig-sandbox/bug-reports/go-swig/D-flag-propagation
clang++ -I . -fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=$WORK=/tmp/go-build -gno-record-gcc-switches -fno-common -o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_all.o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_export.o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/package.cgo2.o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_var_folders_m6_8y6rbg4s2td16l0z7f1pm1_c0000gn_T_go-build554145057_swig-sandbox_bug-reports_go-swig_D-flag-propagation__obj_bug_D_prop.cgo2.o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_var_folders_m6_8y6rbg4s2td16l0z7f1pm1_c0000gn_T_go-build554145057_swig-sandbox_bug-reports_go-swig_D-flag-propagation__obj_bug_D_prop_wrap.cxx.o -g -O2 -Wl,-r -nostdlib
/usr/local/go/pkg/tool/darwin_amd64/compile -o $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation.a -trimpath $WORK -p swig-sandbox/bug-reports/go-swig/D-flag-propagation -buildid 4e1ac2c17dd75d5a2b4901f2a23ecef752660aab -D _/Users/hormozzarnani/go/src/swig-sandbox/bug-reports/go-swig/D-flag-propagation -I $WORK -pack $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_gotypes.go $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/package.cgo1.go $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_var_folders_m6_8y6rbg4s2td16l0z7f1pm1_c0000gn_T_go-build554145057_swig-sandbox_bug-reports_go-swig_D-flag-propagation__obj_bug_D_prop.cgo1.go $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_cgo_import.go
pack r $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation.a $WORK/swig-sandbox/bug-reports/go-swig/D-flag-propagation/_obj/_all.o # internal
bash-3.2$ echo $?
0
bash-3.2$

[bradfitz]

I suspect this is working as intended. @ianlancetaylor?

[hzarnani]

My understanding is that building a go package containing *.swigcxx is functionally equivalent to when the build is done in two steps -- by first manually invoking swig on the interface files followed by running go build (cgo) on the generated wrappers (and without the *.swigcxx files, of course). At least that's the desired behavior. If that is not the case, I believe it warrants documentation.

I have another counter example to the aforementioned desired behavior having to do with the use of swig's %go_import directive, which I'll submit separately.

[ianlancetaylor]

I don't think there are any current guidelines for mixing cgo and SWIG in the same package. It's not obvious why the suggestion is particularly useful--you could add a #define in the .swigcxx file instead. But we should decide how these are supposed to interact.

[hzarnani]

I'm perplexed, to say the least, by the suggestive dismissal of this. One may as well say "what's the utility of the -D flag in gcc? You could just define your symbol in the file itself." Well, for many years, programmers have used -D at the command line to do conditional compilation, among others. The point is that the build envrionemt should have the ability to control what's compiled and how without source code modification (inserting a "#define"). In fact, most sizable and serious C/C++ projects use it. While this particular example is very much of a contrived one, it shows that not all preprocessor flags are propagated to swig, yet they're supposed to. At least it's not unreasonable to assume that they should. And if not, it must be documented which cgo CPPFLAGS/CXXFLAGS are propagated and which ones are striped out.

[rsc]

I too am perplexed as to why we'd blindly pass all the CPPFLAGS to the swig invocation. Does swig take all the same flags as g++?

[ianlancetaylor]

CPPFLAGS is normally used for flags that are passed to the C pre-processor, which is normally just -I and -D flags. (Since you ask about g++ you may be thinking of CXXFLAGS, which are C++ flags passed to g++). SWIG does accept -I and D flags, so in some sense it would be meaningful to pass the options to SWIG. However, -D for SWIG defines macros in the .swig file, and -D for C/C++ obviously defines macros in the .c/.cc files, so they have somewhat different meanings and uses. Similarly, -I for SWIG tells it where to find SWIG include files for the %include directive as well as C/C++ header files for the #include directive. So it's really not clear to me that passing CPPFLAGS to SWIG makes sense in general.

[hzarnani]

I'm not sure why this is perplexing or confusing. It's not uncommon to want to pass flags uninterpreted to a lower-layer tool. The use case I wrote above for conditional compilation is a prime example.

[ianlancetaylor]

We are already passing flags uninterpreted to a lower-layer tool. The question is whether it makes sense to pass the same flags to two different tools for which the flags have similar but clearly different meanings.

[rsc]

If we need to introduce SWIGFLAGS, maybe we should have that discussion. I agree with Ian that it's confusing to repurpose CPPFLAGS for passing flags to SWIG.

[rsc]

It seems clear we are not going to send CPPFLAGS to SWIG.

I don't know whether the need for SWIG flags is common enough to introduce SWIGFLAGS.

[ianlancetaylor]

I think the decision here is whether to add SWIGFLAGS. I don't think we have enough information for how useful that would be.

[rsc]

Given the lack of a compelling argument in favor, closing this.