code review 4287056: Make.pkg: add support for SWIG, and add two SWIG examples (issue4287056)

41 views
Skip to first unread message

ia...@golang.org

unread,
Mar 16, 2011, 10:56:24 PM3/16/11
to r...@golang.org, r...@golang.org, golan...@googlegroups.com, re...@codereview.appspotmail.com
Reviewers: r, rsc,

Message:
Hello r, rsc (cc: golan...@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
Make.pkg: add support for SWIG, and add two SWIG examples

The SWIG examples are not yet built or tested by default.

Please review this at http://codereview.appspot.com/4287056/

Affected files:
A misc/swig/callback/Makefile
A misc/swig/callback/callback.h
A misc/swig/callback/callback.swigcxx
A misc/swig/callback/run
A misc/swig/callback/run.go
A misc/swig/stdio/Makefile
A misc/swig/stdio/file.swig
A misc/swig/stdio/hello
A misc/swig/stdio/hello.go
M src/Make.pkg


r...@golang.org

unread,
Mar 17, 2011, 1:26:32 AM3/17/11
to ia...@golang.org, r...@golang.org, golan...@googlegroups.com, re...@codereview.appspotmail.com
LGTM

http://codereview.appspot.com/4287056/diff/4001/misc/swig/callback/run.go
File misc/swig/callback/run.go (right):

http://codereview.appspot.com/4287056/diff/4001/misc/swig/callback/run.go#newcode8
misc/swig/callback/run.go:8: "swig/callback"
sort

http://codereview.appspot.com/4287056/diff/4001/src/Make.pkg
File src/Make.pkg (right):

http://codereview.appspot.com/4287056/diff/4001/src/Make.pkg#newcode48
src/Make.pkg:48: INSTALLFILES+=$(patsubst
%.swig,$(pkgdir)/$(dir)/%.so,$(patsubst %.swigcxx,%.swig,$(SWIGFILES)))
What do we need to do to get rid of these?
When cgo installed these kinds of things it
did it to a path without /s in them so that
the usual relative lookup could find them
after being copied elsewhere. Do we need to
worry about that here?

http://codereview.appspot.com/4287056/diff/4001/src/Make.pkg#newcode119
src/Make.pkg:119: ifneq ($(CGOFILES)$(SWIGFILES),)
:-)

http://codereview.appspot.com/4287056/diff/4001/src/Make.pkg#newcode195
src/Make.pkg:195: # SWIGFILES=myclib.swig mycxxlib.swigcxx
add
#
# Packages using the resulting binary will refer to
# $(pkgdir)/$target.so and will need that shared object
# at run time.

http://codereview.appspot.com/4287056/

Ian Lance Taylor

unread,
Mar 17, 2011, 11:52:15 AM3/17/11
to r...@golang.org, r...@golang.org, golan...@googlegroups.com, re...@codereview.appspotmail.com
r...@golang.org writes:

> http://codereview.appspot.com/4287056/diff/4001/src/Make.pkg#newcode48
> src/Make.pkg:48: INSTALLFILES+=$(patsubst
> %.swig,$(pkgdir)/$(dir)/%.so,$(patsubst %.swigcxx,%.swig,$(SWIGFILES)))
> What do we need to do to get rid of these?

We need to have a linker which can fully support C++. I think the
simpler long-term path going forward will be to be to have 6l/8l emit an
ELF .o file, and complete the link using ld/. The cost is a two stage
link process when using SWIG. Alternatively we have to teach 6l/8l
about linker scripts, symbol visibility, versioning, and .init section
sorting, all of which is possible but I think more work.

I know less about Mach-O though the basic idea of emitting a Mach-O .o
file will work too.


> When cgo installed these kinds of things it
> did it to a path without /s in them so that
> the usual relative lookup could find them
> after being copied elsewhere. Do we need to
> worry about that here?

Unless I misunderstand I think we're OK. We put a simple name, not a
path, in the DT_NEEDED tag in the executable, so if you move the shared
library you can set LD_LIBRARY_PATH and the executable will find it.
It's not perfect but it's workable.

Ian

Russ Cox

unread,
Mar 17, 2011, 12:11:42 PM3/17/11
to Ian Lance Taylor, r...@golang.org, golan...@googlegroups.com, re...@codereview.appspotmail.com
> Unless I misunderstand I think we're OK.  We put a simple name, not a
> path, in the DT_NEEDED tag in the executable, so if you move the shared
> library you can set LD_LIBRARY_PATH and the executable will find it.
> It's not perfect but it's workable.

I looked again. It looks like the name is foo, so if there are two
different packages x/foo and y/foo they will both refer to foo.so.
Is that true? I think cgo, before it stopped using shared libraries,
referred to x_foo.so and y_foo.so, but I might be misremembering.

Russ

r...@golang.org

unread,
Mar 17, 2011, 8:22:34 PM3/17/11
to ia...@golang.org, r...@golang.org, ia...@google.com, golan...@googlegroups.com, re...@codereview.appspotmail.com

Ian Lance Taylor

unread,
Mar 21, 2011, 5:16:24 PM3/21/11
to r...@golang.org, r...@golang.org, golan...@googlegroups.com, re...@codereview.appspotmail.com
Russ Cox <r...@golang.org> writes:

Good point. Fixed to include the full target name in the .so name.

PTAL.

Ian

r...@golang.org

unread,
Mar 22, 2011, 2:32:51 PM3/22/11
to ia...@golang.org, r...@golang.org, ia...@google.com, golan...@googlegroups.com, re...@codereview.appspotmail.com

ia...@golang.org

unread,
Mar 22, 2011, 4:06:03 PM3/22/11
to ia...@golang.org, r...@golang.org, r...@golang.org, ia...@google.com, golan...@googlegroups.com, re...@codereview.appspotmail.com
*** Submitted as
http://code.google.com/p/go/source/detail?r=53d99c6f0ef8 ***

Make.pkg: add support for SWIG, and add two SWIG examples

The SWIG examples are not yet built or tested by default.

R=r, rsc, iant2
CC=golang-dev
http://codereview.appspot.com/4287056


http://codereview.appspot.com/4287056/

Reply all
Reply to author
Forward
0 new messages