Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
ghost opened this issue Dec 16, 2016 · 12 comments
Closed

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

ghost opened this issue Dec 16, 2016 · 12 comments
Labels
FeatureRequest FrozenDueToAge GoCommand cmd/go NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unplanned

Comments

@ghost
Copy link

ghost commented Dec 16, 2016 •
edited by 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$
@ghost ghost changed the title go build does not propagate preprocessor flag -D from CPP_FLAGS in invoking swig go build does not propagate preprocessor flag -D from CPPFLAGS in invoking swig Dec 16, 2016
@bradfitz
Copy link
Contributor

I suspect this is working as intended. @ianlancetaylor?

@hzarnani
Copy link

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
Copy link
Contributor

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.

@ianlancetaylor ianlancetaylor changed the title go build does not propagate preprocessor flag -D from CPPFLAGS in invoking swig cmd/go: go build does not propagate preprocessor flag -D from CPPFLAGS in invoking swig Dec 16, 2016
@ianlancetaylor ianlancetaylor self-assigned this Dec 16, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.9 milestone Dec 16, 2016
@hzarnani
Copy link

hzarnani commented Dec 16, 2016 •
edited

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.

@hzarnani hzarnani mentioned this issue Dec 16, 2016
Open
@rsc
Copy link
Contributor

rsc commented Jun 22, 2017

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++?

@rsc rsc modified the milestones: Go1.10, Go1.9 Jun 22, 2017
@ianlancetaylor
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

rsc commented Jul 6, 2017

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
Copy link
Contributor

rsc commented Dec 1, 2017

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.

@rsc rsc modified the milestones: Go1.10, Go1.11 Dec 1, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jul 6, 2018
@ianlancetaylor ianlancetaylor added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed Documentation labels Jul 6, 2018
@ianlancetaylor ianlancetaylor removed their assignment Jul 6, 2018
@ianlancetaylor
Copy link
Contributor

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

@bcmills bcmills changed the title cmd/go: go build does not propagate preprocessor flag -D from CPPFLAGS in invoking swig cmd/go: add SWIGFLAGS environment variable to propagate '-D' flags Nov 13, 2018
@bcmills bcmills modified the milestones: Go1.12, Unplanned Nov 13, 2018
@bcmills bcmills added the GoCommand cmd/go label Nov 13, 2018
@rsc
Copy link
Contributor

rsc commented Nov 20, 2018

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

@rsc rsc closed this as completed Nov 20, 2018
@golang golang locked and limited conversation to collaborators Nov 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FeatureRequest FrozenDueToAge GoCommand cmd/go NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@bradfitz @rsc @ianlancetaylor @bcmills @gopherbot @hzarnani