code review 96320043: cmd/go: SWIG #go pseudo directives to tweak compiler be...

cmd/go: SWIG #go pseudo directives to tweak compiler behavior.

This CL has the go tool parse .swig and .swigcxx files to look for
comments at the top of the file containing CGO-like pseudo directives in
the form of:
// #go LDFLAGS: -lpng

The comments may only be preceded by other single-line (not multiline)
comments. This is to simplify the parser implementation. The pseudo
directives added by this CL are all of the CGO ones (CFLAGS, CPPFLAGS,
CXXFLAGS, LDFLAGS, and even pkg-config).

cmd/go/doc.go is modified to have an explicit section on the go tool's
SWIG integration reflecting these changes.

A simple example is added at misc/swig/cflags which demonstrates using a
CFLAGS #go pseudo directive to define a generic value (additionally it
serves as a test because it will not compile if the parser implementation
does not work).

The parsing itself occurs in the go/build package, this CL inherently adds
public SwigCFLAGS etc. fields to the Package struct.

Discussion: https://groups.google.com/forum/#!topic/golang-dev/B_gY96GKVc8
Fixes issue 6696.

[slimsag, 2014-05-13 11:12:44]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[ioe, 2014-05-13 17:34:56]

May I suggest to use '#cgo' as the tag?
Rationale: It is actually the cgo tool that needs those flags.

That allows to add go specific flags (e.g. build tags) if they are needed
later.

On Tuesday, May 13, 2014 1:12:44 PM UTC+2, stephen....@gmail.com wrote:
> Reviewers: golang-codereviews,
> 
> 
> 
> Message:
> 
> Hello golang-codereviews@googlegroups.com,
> 
> 
> 
> I'd like you to review this change to
> 
> https://code.google.com/p/go
> 
> 
> 
> 
> 
> Description:
> 
> cmd/go: SWIG #go pseudo directives to tweak compiler behavior.
> 
> 
> 
> This CL has the go tool parse .swig and .swigcxx files to look for
> 
> comments at the top of the file containing CGO-like pseudo directives in
> 
> the form of:
> 
> // #go LDFLAGS: -lpng
> 
> 
> 
> The comments may only be preceded by other single-line (not multiline)
> 
> comments. This is to simplify the parser implementation. The pseudo
> 
> directives added by this CL are all of the CGO ones (CFLAGS, CPPFLAGS,
> 
> CXXFLAGS, LDFLAGS, and even pkg-config).
> 
> 
> 
> cmd/go/doc.go is modified to have an explicit section on the go tool's
> 
> SWIG integration reflecting these changes.
> 
> 
> 
> A simple example is added at misc/swig/cflags which demonstrates using a
> 
> CFLAGS #go pseudo directive to define a generic value (additionally it
> 
> serves as a test because it will not compile if the parser
> 
> implementation
> 
> does not work).
> 
> 
> 
> The parsing itself occurs in the go/build package, this CL inherently
> 
> adds
> 
> public SwigCFLAGS etc. fields to the Package struct.
> 
> 
> 
> Discussion:
> 
> https://groups.google.com/forum/#!topic/golang-dev/B_gY96GKVc8
> 
> Fixes issue 6696.
> 
> 
> 
> Please review this at https://codereview.appspot.com/96320043/
> 
> 
> 
> Affected files (+299, -30 lines):
> 
>    A misc/swig/cflags/file.c
> 
>    A misc/swig/cflags/file.swig
> 
>    A misc/swig/cflags/file_test.go
> 
>    M src/cmd/go/build.go
> 
>    M src/cmd/go/doc.go
> 
>    M src/cmd/go/help.go
> 
>    M src/cmd/go/list.go
> 
>    M src/cmd/go/main.go
> 
>    M src/cmd/go/pkg.go
> 
>    M src/pkg/go/build/build.go

[slimsag, 2014-05-13 21:16:31]

On 2014/05/13 17:34:56, ioe wrote:
> May I suggest to use '#cgo' as the tag?
> Rationale: It is actually the cgo tool that needs those flags.
> 
> That allows to add go specific flags (e.g. build tags) if they are needed
> later.
> 
> On Tuesday, May 13, 2014 1:12:44 PM UTC+2, mailto:stephen....@gmail.com wrote:

I am not strongly opposed to either way if someone else would like to chime in.
Ian recommended the #go tag when I had originally used #swig instead.

It is not actually the cgo tool that needs those flags at all, they need to be
passed onto gcc directly when building the wrapper files and any C/C++ files in
the package directory. Although SWIG uses the runtime's CGO (cgocall I think)
code, the go tool does not even invoke the CGO tool for a swig project as far as
I know.

Stephen

[ioe, 2014-05-13 22:46:00]

Ok, I take back my suggestion. Your arguments make a lot of sense to me.

On Tuesday, May 13, 2014 11:16:30 PM UTC+2, stephen....@gmail.com wrote:
> On 2014/05/13 17:34:56, ioe wrote:
> 
> > May I suggest to use '#cgo' as the tag?
> 
> > Rationale: It is actually the cgo tool that needs those flags.
> 
> 
> 
> > That allows to add go specific flags (e.g. build tags) if they are
> 
> needed
> 
> > later.
> 
> 
> 
> > On Tuesday, May 13, 2014 1:12:44 PM UTC+2,
> 
> mailto:stephen....@gmail.com wrote:
> 
> 
> 
> I am not strongly opposed to either way if someone else would like to
> 
> chime in. Ian recommended the #go tag when I had originally used #swig
> 
> instead.
> 
> 
> 
> It is not actually the cgo tool that needs those flags at all, they need
> 
> to be passed onto gcc directly when building the wrapper files and any
> 
> C/C++ files in the package directory. Although SWIG uses the runtime's
> 
> CGO (cgocall I think) code, the go tool does not even invoke the CGO
> 
> tool for a swig project as far as I know.
> 
> 
> 
> Stephen
> 
> 
> 
> https://codereview.appspot.com/96320043/

[iant, 2014-05-14 13:29:04]

I think this is a good idea but it's clearly a new feature and we are well into
our feature freeze.  Let's take it up again after 1.3 is released.

[slimsag, 2014-05-14 21:20:02]

On 2014/05/14 13:29:04, iant wrote:
> I think this is a good idea but it's clearly a new feature and we are well
into
> our feature freeze.  Let's take it up again after 1.3 is released.

SGTM. Wasn't sure at the time if it was considered a bug or features.

I will resubmit after Go 1.3 is out.

[slimsag, 2014-06-19 23:46:36]

On 2014/05/14 21:20:02, slimsag wrote:
> On 2014/05/14 13:29:04, iant wrote:
> > I think this is a good idea but it's clearly a new feature and we are well
> into
> > our feature freeze.  Let's take it up again after 1.3 is released.
> 
> SGTM. Wasn't sure at the time if it was considered a bug or features.
> 
> I will resubmit after Go 1.3 is out.

For anyone curious about this:

I'm in the process of merging this CL with recent cmd/go and go/build changes.

I found that the patches here did not work as well as I thought they did (I'm
working on this):
    1. It feeds the LDFLAGS into gcc when building a package -- but not when
building the executable (a main package).
    2. I found a small mistaken assumption about the line length when checking
if a line is a comment ("//") which could've caused a panic.

Stephen

[slimsag, 2014-06-20 02:50:43]

On 2014/06/19 23:46:36, slimsag wrote:
> On 2014/05/14 21:20:02, slimsag wrote:
> > On 2014/05/14 13:29:04, iant wrote:
> > > I think this is a good idea but it's clearly a new feature and we are well
> > into
> > > our feature freeze.  Let's take it up again after 1.3 is released.
> > 
> > SGTM. Wasn't sure at the time if it was considered a bug or features.
> > 
> > I will resubmit after Go 1.3 is out.
> 
> For anyone curious about this:
> 
> I'm in the process of merging this CL with recent cmd/go and go/build changes.
> 
> I found that the patches here did not work as well as I thought they did (I'm
> working on this):
>     1. It feeds the LDFLAGS into gcc when building a package -- but not when
> building the executable (a main package).
>     2. I found a small mistaken assumption about the line length when checking
> if a line is a comment ("//") which could've caused a panic.
> 
> Stephen

I've fixed the aforementioned issues with this CL.

PTAL

[michael-schaller, 2014-11-05 09:34:04]

@slimlang
Can you also add general SWIG options? I would like to run SWIG with the -Wall
and -Werror options.


I've also tried using your patch. The patch itself applied cleanly to Go 1.3.2
from Debian Jessie. I've tried then to use the patched Go version as is with an
existing and yet unmodified project and 'go test' failed with an 'index out of
range' error that does not appear with the unpatched version:

$ CC='g++ -l<library>' go test
panic: runtime error: index out of range

goroutine 22 [running]:
runtime.panic(0x7fa440, 0xa8173c)
	/usr/lib/go/src/pkg/runtime/panic.c:279 +0xf5
main.gcToolchain.ld(0xc208062460, 0xc2080cb680, 0xc2080fe380, 0x31,
0xc208089e00, 0x1d, 0x20, 0xc2081488a0, 0x2b, 0x0, ...)
	/usr/lib/go/src/cmd/go/build.go:1760 +0x1005
main.gcToolchain.ld·i(0x0, 0xc208062460, 0xc2080cb680, 0xc2080fe380, 0x31,
0xc208089e00, 0x1d, 0x20, 0xc2081488a0, 0x2b, ...)
	<autogenerated>:15 +0xe3
main.(*builder).build(0xc208062460, 0xc2081008f0, 0x0, 0x0)
	/usr/lib/go/src/cmd/go/build.go:1013 +0x3953
main.func·005(0xc2081008f0)
	/usr/lib/go/src/cmd/go/build.go:694 +0x38e
main.func·006()
	/usr/lib/go/src/cmd/go/build.go:751 +0x12b
created by main.(*builder).do
	/usr/lib/go/src/cmd/go/build.go:757 +0x44c


I can attach a stripped down example to issue 6696 to reproduce this issue if
needed.

[slimsag, 2014-11-05 13:43:10]

The CL probably doesn't apply well to tip, surely a lot has changed since then. 

No comments for ~4 months, I assumed others were not very interested. It's no
longer a priority for me.

If it's needed by you I can work on updating it sometime before the Go 1.5
feature freeze (since Go 1.4 is in it's feature freeze).

- Stephen

On 2014/11/05 09:34:04, misch wrote:
> @slimlang
> Can you also add general SWIG options? I would like to run SWIG with the -Wall
> and -Werror options.
> 
> 
> I've also tried using your patch. The patch itself applied cleanly to Go 1.3.2
> from Debian Jessie. I've tried then to use the patched Go version as is with
an
> existing and yet unmodified project and 'go test' failed with an 'index out of
> range' error that does not appear with the unpatched version:
> 
> $ CC='g++ -l<library>' go test
> panic: runtime error: index out of range
> 
> goroutine 22 [running]:
> runtime.panic(0x7fa440, 0xa8173c)
> 	/usr/lib/go/src/pkg/runtime/panic.c:279 +0xf5
> main.gcToolchain.ld(0xc208062460, 0xc2080cb680, 0xc2080fe380, 0x31,
> 0xc208089e00, 0x1d, 0x20, 0xc2081488a0, 0x2b, 0x0, ...)
> 	/usr/lib/go/src/cmd/go/build.go:1760 +0x1005
> main.gcToolchain.ld·i(0x0, 0xc208062460, 0xc2080cb680, 0xc2080fe380, 0x31,
> 0xc208089e00, 0x1d, 0x20, 0xc2081488a0, 0x2b, ...)
> 	<autogenerated>:15 +0xe3
> main.(*builder).build(0xc208062460, 0xc2081008f0, 0x0, 0x0)
> 	/usr/lib/go/src/cmd/go/build.go:1013 +0x3953
> main.func·005(0xc2081008f0)
> 	/usr/lib/go/src/cmd/go/build.go:694 +0x38e
> main.func·006()
> 	/usr/lib/go/src/cmd/go/build.go:751 +0x12b
> created by main.(*builder).do
> 	/usr/lib/go/src/cmd/go/build.go:757 +0x44c
> 
> 
> I can attach a stripped down example to issue 6696 to reproduce this issue if
> needed.

[michael-schaller, 2014-11-05 15:05:45]

Stephen, I would be very happy if you could do that. The code looks good to me
and maybe the issue I'm seeing is going away as soon as the code has been
adapted to head. I'm certainly more than happy to test a new revision of your
CL.

On 2014/11/05 13:43:10, slimsag wrote:
> The CL probably doesn't apply well to tip, surely a lot has changed since
then. 
> 
> No comments for ~4 months, I assumed others were not very interested. It's no
> longer a priority for me.
> 
> If it's needed by you I can work on updating it sometime before the Go 1.5
> feature freeze (since Go 1.4 is in it's feature freeze).
> 
> - Stephen

[gobot, 2014-12-19 05:19:21]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/96320043 is best) in the description in your
new CL.

Thanks very much.