code review 3921043: cgo: CGO_CFLAGS and CGO_LDFLAGS support

cgo: CGO_CFLAGS and CGO_LDFLAGS support

This is a proposal to enable cgo to support a way of
defining CGO_CFLAGS and CGO_LDFLAGS.

[niemeyer, 2011-01-11 21:11:55]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[niemeyer, 2011-01-12 20:18:18]

There's a somewhat obvious problem with this approach which is
a chicken and egg issue: cgo will run gcc to figure the defines,
but CFLAGS may affect the ability of cgo to produce correct
gcc output.

Still pondering about a good alternative to avoid the issue with
this same approach, so feel free to ignore this review for the
moment.

[niemeyer, 2011-01-12 23:33:40]

Ok, problem solved by running a first pass of gcc with the -M -MG
options, so that it reports unavailable headers rather than failing.
With this change cgo itself will also run gcc with the flags provided
via CGO_CFLAGS.

With this change in place, you can try goinstall of a cgo package with the
following repository:

    goinstall github.com/niemeyer/goyaml

If this implementation isn't exactly what you have in mind, it
should be heading in the proper direction.

Please let me know what you think.

[rsc, 2011-01-13 00:06:11]

I don't want to have to invoke the C compiler to find out the flags.
I would suggest making them separate comments.

//cgo CFLAGS: -I/usr/local/lib
//cgo LDFLAGS: -L/usr/local/lib -lX11

or something like that.  Designing those is the question.
Leaving it to people to do preprocessor magic to compute
them definitely seems like asking for trouble.

Russ

[niemeyer, 2011-01-13 00:47:59]

> I don't want to have to invoke the C compiler to find out the flags.
> I would suggest making them separate comments.
(...)


That's certainly simpler, and given what's already in place,
should be easy to support something along those lines. I'll
think further about the problem and get a prototype in place.

For the record, patch set 4 is the last one using the
preprocessor magic.

[niemeyer, 2011-01-13 04:25:33]

Implemented an approach similar to what was suggested. The
proposed syntax is:

#cgo CFLAGS: -I/usr/include/X11

The prefix looks like a preprocessor directive, which besides
integrating better with C style, ensures the line can't
conflict with any real code.  Using : as a separator
was a good idea and preserved, since the command line may
contain embedded quotes which will be processed correctly.

Flags will be extracted out of all .go files and concatenated
together, so that they may be defined closer to the logic
which they are related to, and so that moving a file across
packages will work better.  Flags across all files are processed
before any translation starts, so that gcc is always run the
same way, with all defined flags.

The sample package at github.com/niemeyer/goyaml was updated to
support the new syntax.

PTAL

[rsc, 2011-01-13 15:20:26]

On Wed, Jan 12, 2011 at 23:25,  <n13m3y3r@gmail.com> wrote:
> Implemented an approach similar to what was suggested. The
> proposed syntax is:
>
> #cgo CFLAGS: -I/usr/include/X11

My proposal was to put the comment anywhere in the file,
not inside the C code.  But I think this is okay.

> Flags will be extracted out of all .go files and concatenated
> together, so that they may be defined closer to the logic
> which they are related to, and so that moving a file across
> packages will work better.

Doesn't cgo invoke gcc separately for each file?
Why should the flags be concatenated?

Russ

[niemeyer, 2011-01-13 15:51:59]

> Doesn't cgo invoke gcc separately for each file?
> Why should the flags be concatenated?

With this approach I had in mind:

- When there are external .c files to be built with gcc, it's
  not clear which flags will be used otherwise.  This behavior
  makes it consistent and easy to understand and use.

- The opposite behavior requires repeating CFLAGS on every
  file of a package split in multiple files importing from
  C for organization, which feels repetitive and error prone.

- Linking has to concatenate LDFLAGS to work

- People are used to passing CFLAGS/LDFLAGS globally to a
  build procedure.

Based on these, does it feel like a sane decision?

[rsc, 2011-01-13 17:57:20]

Ok.

[niemeyer, 2011-01-17 09:14:21]

Improved SplitQuoted function and submitted it for inclusion into
the strings package at http://codereview.appspot.com/4051041

If you're happy with the approach, I believe this is close
to being ready for inclusion now.

[binet, 2011-01-28 09:20:12]

On 2011/01/17 09:14:21, niemeyer wrote:
> Improved SplitQuoted function and submitted it for inclusion into
> the strings package at http://codereview.appspot.com/4051041
> 
> If you're happy with the approach, I believe this is close
> to being ready for inclusion now.

I tested this diff and it works great for most of my cgo packages (thanks!)

but what about flags which are derived from e.g. pkg-config like binaries or
scripts ?

such an example is my go-python package:
 http://bitbucket.org/binet/go-python/src/tip/pkg/Makefile

[niemeyer, 2011-01-28 15:51:26]

Yes, pkg-config is an interesting example.  For that to work, we'll
either have to parse $() style arguments out of the string, or will
have to pass the arguments to the shell without splitting.  The latter option
has the advantage that we may avoid getting into arg splitting
business entirely, as is being done now. I will give that a try to
see how it feels.

[rsc, 2011-01-31 20:06:13]

On Fri, Jan 28, 2011 at 10:51,  <n13m3y3r@gmail.com> wrote:
> Yes, pkg-config is an interesting example.  For that to work, we'll
> either have to parse $() style arguments out of the string, or will
> have to pass the arguments to the shell without splitting.  The latter
> option has the advantage that we may avoid getting into arg splitting
> business entirely, as is being done now. I will give that a try to
> see how it feels.

I don't want to have shell interpretation for now.
Let's get basic things working.  It doesn't bother me
if people who want to use pkgconfig have to write
Makefiles still.

Russ

[niemeyer, 2011-01-31 20:23:39]

> I don't want to have shell interpretation for now.
> Let's get basic things working.  It doesn't bother me
> if people who want to use pkgconfig have to write
> Makefiles still.

Ok.  Can I do anything else to push this forward?

[rsc, 2011-01-31 20:31:20]

s/SplitQuoted/splitQuoted/
please sync and upload to incorporate 
recent changes to cgo.

http://codereview.appspot.com/3921043/diff/39001/src/Make.pkg
File src/Make.pkg (right):

http://codereview.appspot.com/3921043/diff/39001/src/Make.pkg#newcode116
src/Make.pkg:116: CGOPKGPATH=$(dir) cgo -- $(CGO_CFLAGS) $(CGOFILES)
This doesn't look right.
I think _cgo_defun.c should stay as it was.

And then here there should be a top level line
right after ifdef CGOFILES that says

$(eval CGOPKGPATH=$(dir) cgo -print-vars -- $(CGO_FLAGS) $(CGOFILES))

Otherwise, if _cgo_defun.c already exists the needed
variables will not be loaded into the makefile.

[niemeyer, 2011-01-31 21:23:39]

Good point about the processing of _cgo_flags, sorry
about that.

If feasible, I would prefer to maintain the current
mechanism of using _cgo_flags because it avoids
processing all cgo files on every make command no
matter what is being done (e.g. make clean).

I think I've solved the described problem without
incurring in the mentioned disadvantage.  Please
take a look and let me know what you think.

[rsc, 2011-01-31 23:48:45]

http://codereview.appspot.com/3921043/diff/17002/src/Make.pkg
File src/Make.pkg (right):

http://codereview.appspot.com/3921043/diff/17002/src/Make.pkg#newcode120
src/Make.pkg:120: $(eval include _cgo_flags)
this is a strange rule.
i'd move the eval into the _cgo_flags rule, and then 
move _cgo_defun.c to the # Ugly but necessary rule.

[niemeyer, 2011-02-01 00:28:54]

> this is a strange rule.
> i'd move the eval into the _cgo_flags rule, and then 
> move _cgo_defun.c to the # Ugly but necessary rule.

There's an ordering constraint which we must respect:
the _cgo_flags inclusion can't happen in the same target
which generates the _cgo_flags file, or it'd include
the file from the previous build.

To improve the situation, I have commented about the
constraint, and have clarified the target names by
introducing a timestamp file for the cgo run.

Please see if you prefer this way or would rather use
one of the files generated by cgo to avoid the extra
timestamp file (with a custom name we can't do without
one or the rule would always execute).

[niemeyer, 2011-02-01 00:30:51]

> There's an ordering constraint which we must respect:
> the _cgo_flags inclusion can't happen in the same target
> which generates the _cgo_flags file, or it'd include
> the file from the previous build.

Just for clarity here, there's no magic there.  The inclusion
happens with a macro, and macros are expanded when the rule
is evaluated.  If we had both in the same rule, the $(eval ...)
would be evaluated in the same moment the $(CGO_CFLAGS) is
evaluated to execute cgo, so naturally it'd happen before cgo
had a chance to generate the file we want to include.

[rsc, 2011-02-01 13:15:46]

> Just for clarity here, there's no magic there.  

Oh, there's magic here.

:-)

http://codereview.appspot.com/3921043/diff/17008/src/Make.pkg
File src/Make.pkg (right):

http://codereview.appspot.com/3921043/diff/17008/src/Make.pkg#newcode120
src/Make.pkg:120: # The inclusion happens when the macro is expanded, so it
can't be done within
# The include happens before the commands in the recipe run,
# so it cannot be done in the same recipe that runs cgo.

[niemeyer, 2011-02-01 13:18:48]

> Oh, there's magic here.

Yeah, after I mentioned it I was pondering about what's the
meaning of magic as well. :-)

> # The include happens before the commands in the recipe run,
> # so it cannot be done in the same recipe that runs cgo.

Done. PTAL

[rsc, 2011-02-01 13:33:19]

LGTM

Okay, the next thing we need in order to make this useful
with goinstall is to be able to specify the CFLAGS and LDFLAGS
on a per-OS basis.   The current syntax is

#cgo name: value

I suggest changing it to allow an optional system constraint
of the form $GOOS, $GOARCH, or $GOOS/$GOARCH before
the name, as in

#cgo darwin CFLAGS: xxx
#cgo 386 CFLAGS: xxx
#cgo darwin/386 CFLAGS: xxx

Given that I think we can get basic cgo packages like gosqlite
working with goinstall.

Russ

[rsc, 2011-02-01 13:33:43]

(But that suggestion is for a different CL.)

[rsc, 2011-02-01 13:44:22]

*** Submitted as fd915bcad5d5 ***

cgo: define CGO_CFLAGS and CGO_LDFLAGS in Go files

R=rsc, binet
CC=golang-dev
http://codereview.appspot.com/3921043

Committer: Russ Cox <rsc@golang.org>

[niemeyer, 2011-02-01 13:49:11]

> I suggest changing it to allow an optional system constraint
> of the form $GOOS, $GOARCH, or $GOOS/$GOARCH before
> the name, as in

That sounds good.  I'll work on it.