diff mbox

PATCH RFA: Add Go support to top level configure and Makefiles

Message ID mcr8w0pawpe.fsf@google.com
State New
Headers show

Commit Message

Ian Lance Taylor Nov. 19, 2010, 8:24 p.m. UTC 
Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

>> +# If target-libgo is in $target_configdirs, then put target-libffi in
>> +# there too.  It may have been removed because it is currently treated
>> +# as a Java target library.
>> +if echo " ${target_configdirs} " | grep " target-libgo " >/dev/null 2>&1 \
>> +   && test -f $srcdir/libgo/configure -a -f $srcdir/libffi/configure; then
>
> Prefer '&& test' instead of -a.
>
> Conceptually, this code is the wrong thing to do: it should not happen
> after the $skipdirs and $noconfigdirs elimination.  That may not have
> any impact on the systems that gccgo currently works on, but the code
> might be ported; and the snippet may set a precedent for other code.
>
> I think the logic could come shortly before the noconfigdirs
> elimination.

Thanks for the review.

Does this version (change to configure.ac only) look better to you?


> Aside, I see that libgo/configure handles --disable-libffi.  Since this
> option is also used at the toplevel to disable the libffi target
> directories, that seems to imply that, if used, it is always an in-tree
> libffi that is used.  Is that correct?

Unless I misunderstand what you are saying, I don't think that is
correct.  The code in libgo/configure.ac is trying to say that if there
is an in-tree libffi, it should be used.  If there isn't an in-tree
libffi, then it will use the system libffi.  If there is neither an
in-tree libffi nor a system libffi, then libgo will fail to build.

Ian

Comments

Ralf Wildenhues Nov. 19, 2010, 8:41 p.m. UTC | #1 
* Ian Lance Taylor wrote on Fri, Nov 19, 2010 at 09:24:13PM CET:
> Does this version (change to configure.ac only) look better to you?

Well, there is one more thing: if target-libffi is already listed, it
will be listed twice now.  You can grep it to avoid duplicate entries.

The patch seems OK to me then, if it works in the following
configurations:
  --enable-languages=c
  --enable-languages=c,go
  --enable-languages=c,go,java

but please leave a bit for other build maintainers to get a chance to
fix my review if need be.  The toplevel changes need to be synced to
src.

> Ralf Wildenhues writes:
> 
> > Aside, I see that libgo/configure handles --disable-libffi.  Since this
> > option is also used at the toplevel to disable the libffi target
> > directories, that seems to imply that, if used, it is always an in-tree
> > libffi that is used.  Is that correct?
> 
> Unless I misunderstand what you are saying, I don't think that is
> correct.  The code in libgo/configure.ac is trying to say that if there
> is an in-tree libffi, it should be used.  If there isn't an in-tree
> libffi, then it will use the system libffi.  If there is neither an
> in-tree libffi nor a system libffi, then libgo will fail to build.

I don't see where libgo/configure.ac errors out when neither in-tree nor
out-of-tree libffi are chosen (rather, the help string says "don't use
libffi").  I'm actually not quite sure whether it is --disable-libffi or
--without-libffi that disables its building (with the directories in
src, --disable is used throughout, but in GCC namings seem to be a bit
inconsistent).  Anyway a patch to merge that would benefit from being
tested in all combinations (in-tree libffi, out-of-tree libffi with
either an in-tree present, or not, and out-of-tree desired but not
present to ensure error).  But that is for another patch to see.

Thanks,
Ralf
Ian Lance Taylor Nov. 19, 2010, 9:15 p.m. UTC | #2 
Ian Lance Taylor <iant@google.com> writes:

> Does this version (change to configure.ac only) look better to you?

Oh, wait, this doesn't work.  The problem is that the Java libraries get
added to noconfigdirs if Java is not enabled.  So I can add libffi
there, but it just gets taken out again.

I think I am going to commit the rest of the patch and figure out how to
handle this issue separately.  The current language support doesn't
handle the case of a target library which is used by two languages.

Ian
Ian Lance Taylor Nov. 19, 2010, 9:20 p.m. UTC | #3 
Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

>> Unless I misunderstand what you are saying, I don't think that is
>> correct.  The code in libgo/configure.ac is trying to say that if there
>> is an in-tree libffi, it should be used.  If there isn't an in-tree
>> libffi, then it will use the system libffi.  If there is neither an
>> in-tree libffi nor a system libffi, then libgo will fail to build.
>
> I don't see where libgo/configure.ac errors out when neither in-tree nor
> out-of-tree libffi are chosen (rather, the help string says "don't use
> libffi").

It doesn't.  What happens is that libgo fails to build.  I suppose it
would be possible to adjust libgo so that if libffi is not available the
relevant functionality (reflect.Call) is not provided.  I don't see that
as high priority--there are many more significant portability issues.

Ian
diff mbox

Patch

Index: configure.ac
===================================================================
--- configure.ac	(revision 166919)
+++ configure.ac	(working copy)
@@ -200,7 +200,8 @@  target_libraries="target-libgcc \
 		target-boehm-gc \
 		${libgcj} \
 		target-libobjc \
-		target-libada"
+		target-libada \
+		target-libgo"
 
 # these tools are built using the target libraries, and are intended to
 # run only in the target environment
@@ -1209,6 +1210,7 @@  if test "${build}" != "${host}" ; then
   CXX_FOR_BUILD=${CXX_FOR_BUILD-g++}
   GCJ_FOR_BUILD=${GCJ_FOR_BUILD-gcj}
   GFORTRAN_FOR_BUILD=${GFORTRAN_FOR_BUILD-gfortran}
+  GOC_FOR_BUILD=${GOC_FOR_BUILD-gccgo}
   DLLTOOL_FOR_BUILD=${DLLTOOL_FOR_BUILD-dlltool}
   LD_FOR_BUILD=${LD_FOR_BUILD-ld}
   NM_FOR_BUILD=${NM_FOR_BUILD-nm}
@@ -1222,6 +1224,7 @@  else
   CXX_FOR_BUILD="\$(CXX)"
   GCJ_FOR_BUILD="\$(GCJ)"
   GFORTRAN_FOR_BUILD="\$(GFORTRAN)"
+  GOC_FOR_BUILD="\$(GOC)"
   DLLTOOL_FOR_BUILD="\$(DLLTOOL)"
   LD_FOR_BUILD="\$(LD)"
   NM_FOR_BUILD="\$(NM)"
@@ -1951,6 +1954,15 @@  case ,${enable_languages},:${enable_objc
     ;;
 esac
 
+# If target-libgo is in $target_configdirs, then put target-libffi in
+# there too.  It may have been removed because it is currently treated
+# as a Java target library.
+if echo " ${target_configdirs} " | grep " target-libgo " >/dev/null 2>&1 \
+   && test -f $srcdir/libgo/configure \
+   && test -f $srcdir/libffi/configure; then
+  target_configdirs="${target_configdirs} target-libffi"
+fi
+
 # Remove the entries in $skipdirs and $noconfigdirs from $configdirs,
 # $build_configdirs and $target_configdirs.
 # If we have the source for $noconfigdirs entries, add them to $notsupp.
@@ -3052,6 +3064,7 @@  AC_SUBST(CXX_FOR_BUILD)
 AC_SUBST(DLLTOOL_FOR_BUILD)
 AC_SUBST(GCJ_FOR_BUILD)
 AC_SUBST(GFORTRAN_FOR_BUILD)
+AC_SUBST(GOC_FOR_BUILD)
 AC_SUBST(LDFLAGS_FOR_BUILD)
 AC_SUBST(LD_FOR_BUILD)
 AC_SUBST(NM_FOR_BUILD)
@@ -3162,6 +3175,7 @@  NCN_STRICT_CHECK_TARGET_TOOLS(CXX_FOR_TA
 NCN_STRICT_CHECK_TARGET_TOOLS(GCC_FOR_TARGET, gcc, ${CC_FOR_TARGET})
 NCN_STRICT_CHECK_TARGET_TOOLS(GCJ_FOR_TARGET, gcj)
 NCN_STRICT_CHECK_TARGET_TOOLS(GFORTRAN_FOR_TARGET, gfortran)
+NCN_STRICT_CHECK_TARGET_TOOLS(GOC_FOR_TARGET, gccgo)
 
 ACX_CHECK_INSTALLED_TARGET_TOOL(AR_FOR_TARGET, ar)
 ACX_CHECK_INSTALLED_TARGET_TOOL(AS_FOR_TARGET, as)
@@ -3192,6 +3206,8 @@  GCC_TARGET_TOOL(gcj, GCJ_FOR_TARGET, GCJ
 		[gcc/gcj -B$$r/$(HOST_SUBDIR)/gcc/], java)
 GCC_TARGET_TOOL(gfortran, GFORTRAN_FOR_TARGET, GFORTRAN,
 		[gcc/gfortran -B$$r/$(HOST_SUBDIR)/gcc/], fortran)
+GCC_TARGET_TOOL(gccgo, GOC_FOR_TARGET, GOC,
+		[gcc/gccgo -B$$r/$(HOST_SUBDIR)/gcc/], go)
 GCC_TARGET_TOOL(ld, LD_FOR_TARGET, LD, [ld/ld-new])
 GCC_TARGET_TOOL(lipo, LIPO_FOR_TARGET, LIPO)
 GCC_TARGET_TOOL(nm, NM_FOR_TARGET, NM, [binutils/nm-new])