Portability fixups #112

pull luke-jr wants to merge 1 commits into bitcoin-core:master from luke-jr:fixups changing 2 files +11 −16

luke-jr commented at 11:40 PM on November 20, 2014: member

No description provided.
luke-jr force-pushed on Nov 20, 2014
sipa commented at 12:55 PM on November 21, 2014: contributor

@theuni comments?

theuni commented at 6:26 AM on November 26, 2014: contributor

@luke-jr please hold off on the pkg-config change for mingw for now. The bitcoin depends assume that pkg-config isn't present, so the paths aren't properly configured. The result will likely be native paths showing up in the cross link. I'm not arguing against the change by any means, we just need to fix the bitcoin behavior at the same time.

As for the dumpmachine change.. I really don't like bypassing autoconf's host strings like that. It just looks like trouble in the future. Could we compromise with a more localized change?:

diff --git a/build-aux/m4/bitcoin_secp.m4 b/build-aux/m4/bitcoin_secp.m4
index 4ca28f9..63cc396 100644
--- a/build-aux/m4/bitcoin_secp.m4
+++ b/build-aux/m4/bitcoin_secp.m4
@@ -11,20 +11,24 @@ fi

 dnl 
 AC_DEFUN([SECP_64BIT_ASM_CHECK],[
-if test x"$host_cpu" == x"x86_64"; then
-  AC_CHECK_PROG(YASM, yasm, yasm)
-else
-  if test x"$set_field" = x"64bit_asm"; then
-    AC_MSG_ERROR([$set_field field support explicitly requested but is not compatible with this host])
-  fi
-fi
+host_abi="`$CC -dumpmachine`"
+case x"$host_abi" in
+  *x86_64*)
+    AC_CHECK_PROG(YASM, yasm, yasm)
+    ;;
+  *)
+    if test x"$set_field" = x"64bit_asm"; then
+      AC_MSG_ERROR([$set_field field support explicitly requested but is not compatible with this host])
+    fi
+    ;;
+esac
 if test x$YASM = x; then
   if test x"$set_field" = x"64bit_asm"; then
     AC_MSG_ERROR([$set_field field support explicitly requested but yasm was not found])
   fi
   has_64bit_asm=no
 else
-  case x"$host_os" in
+  case x"$host_abi" in
   xdarwin*)
     YASM_BINFMT=macho64
     ;;

Other two changes are fine by me.

I'm getting ready to head out of town and won't be able to discuss this for ~1.5 weeks. You're welcome to pull in my suggestion, or use @luke-jr's if he doesn't think that will suffice.

luke-jr commented at 6:35 AM on November 26, 2014: member

pkg-config should never put native headers/libs in the cross-compiling... not sure what problem you're talking about there.

Where does $host_abi come from? I don't see this documented anywhere.
theuni commented at 6:44 AM on November 26, 2014: contributor

host_abi is set via $CC -dumpmachine, see higher up.

pkg-config will return system paths if PKG_CONFIG_PATH/PKG_CONFIG_LIBDIR aren't set to the cross paths.

I just double-checked bitcoin's depends though, and those are actually set properly even though we don't use them. So while that is thus-far untested, it may be ok afterall. Hesitant utACK on that one.
luke-jr commented at 6:53 AM on November 26, 2014: member

Ah, I see. Using "host_abi" is clearly the right thing for YASM_BINFMT as well, though.

The /opt/local/* paths should probably really be only for native builds, so I don't care what we do there (maybe it should explicitly only activate when native?)
theuni commented at 6:59 AM on November 26, 2014: contributor

host_abi is used for YASM_BINFMT as well above. Not sure what you meant by that.

/opt/local has indeed been fixed in master to only activate in the case of osx+native+macports. See e2274c58e66759dd4a9502734c8a289a09e44083 .
sipa commented at 3:25 PM on December 2, 2014: contributor

Most of this is probably unnecessary after #128.
luke-jr force-pushed on Dec 9, 2014
sipa commented at 6:55 PM on February 13, 2015: contributor

@luke-jr Is this still needed?
Use pkg-config always when possible, with failover to manual checks for libcrypto 2b4cf416e7
luke-jr force-pushed on Feb 13, 2015
luke-jr commented at 11:45 PM on February 13, 2015: member

Looks like just the pkg-config stuff is useful now. Rebased
sipa commented at 12:33 AM on February 15, 2015: contributor

@theuni Can you have a look?
theuni commented at 5:58 PM on February 16, 2015: contributor

@luke-jr If we're going to fall back to AC_CHECK_HEADER/AC_CHECK_LIB after the pkg-config check for openssl has failed, we'll need to do a TRY_LINK on it before trusting it. Otherwise, a system lib could be found on a cross build where no pkg-config'd openssl exists, and the final link would fail.

Now that I think about it though, may as well just do a TRY_LINK after pkg-config and the fallback, since pkg-config could feasibly find a system lib as well.

Other than that, looks good.
gmaxwell added this to the milestone initial release on Aug 31, 2015
sipa commented at 3:30 PM on September 4, 2015: contributor

@luke-jr care to have a look at @theuni's suggestion?
luke-jr commented at 4:16 PM on September 4, 2015: member

If AC_* ever pick up the wrong library, that seems a bug in autotools?
theuni commented at 4:32 PM on September 4, 2015: contributor

Yea, never mind that suggestion. Lgtm.
sipa merged this on Sep 4, 2015
sipa closed this on Sep 4, 2015
sipa referenced this in commit 85e3a2cc08 on Sep 4, 2015

Contributors

luke-jr

sipa

theuni

Milestone
stable release (1.0.0-rc.1)

Linked (view graph)

#128 Convert YASM code to inline assembly