Pass LD and LDFLAGS to more dependencies #21764

ararslan · 2017-05-09T23:01:45Z

I noticed that we don't pass the LD and LDFLAGS Make variables to some dependencies. In particular, MPFR can pick up the wrong GMP without giving it LDFLAGS. This PR adds Make and CMake options to various dependencies to ensure more dependencies see the user's choice of linker and linker flags.

staticfloat · 2017-05-09T23:04:10Z

deps/tools/common.mk

@@ -29,6 +31,7 @@ CMAKE_COMMON += -DCMAKE_CXX_COMPILER="$(CXX_BASE)"
 ifneq ($(strip $(CMAKE_CXX_ARG)),)
 CMAKE_COMMON += -DCMAKE_CXX_COMPILER_ARG1="$(CMAKE_CXX_ARG)"
 endif
+CMAKE_COMMON += -DCMAKE_LINKER="$$(which $(LD))"


Why are you using which $(LD) instead of just $(LD)? Also, it's better to just say $(shell which $(LD)) instead of using the $$(...) syntax.

I was just copying the style used several lines up for CC. I'll go with $(LD). Thanks!

some versions of cmake were buggy if you didn't pass an absolute path for CC. not sure about LD, but should test that this doesn't cause any issues with cross compilation

I even left a comment # The call to which here is to work around https://cmake.org/Bug/view.php?id=14366 - that was fixed in cmake 3.0, we require 3.4.3 these days unless you're using system llvm

tkelman · 2017-05-09T23:48:24Z

shouldn't these pass through to submakes if defined and exported?

ararslan · 2017-05-09T23:55:45Z

LD yes, or at least I would have thought so, but I was seeing cases where the default ld was being used, even though I had

override LD := /usr/local/bin/ld
export LD

in my Make.user.

tkelman · 2017-05-09T23:58:56Z

which cases specifically?

ararslan · 2017-05-10T00:03:28Z

LLVM is all I can recall offhand, though there may have been others. (Verbose build output lost to terminal history.)

tkelman · 2017-05-10T00:32:04Z

probably CMAKE_COMMON is the only one that's not redundant then?

staticfloat · 2017-05-10T00:42:57Z

There's a key difference between export LD and passing LD=$(LD) on the command line; the second flavor is more forceful. If the sub-make sets the variable at all (e.g. initializes LD="ld") then exporting doesn't work. Example:

# Makefile
FOO=bar
export FOO

exported:
        @$(MAKE) -C subdir

passed:
        @$(MAKE) -C subdir FOO=$(FOO)

# subdir/Makefile
FOO=qux

all:
        @echo "FOO is $(FOO)"

$ make exported
FOO is qux
$ make passed
FOO is bar

staticfloat · 2017-05-10T00:46:09Z

Personally, I prefer the more explicit (passing) approach as it's easier for me to follow (just a personal preference) and it's more forceful, thereby dealing with a larger array of Makefile authorship styles.

ararslan · 2017-05-12T19:27:03Z

So is this reasonable?

tkelman · 2017-05-12T20:48:52Z

deps/mpfr.mk

@@ -15,6 +15,7 @@ MPFR_OPTS += --disable-thread-safe CFLAGS="$(CFLAGS) -DNPRINTF_L -DNPRINTF_T -DN
 endif
 endif

+MPFR_OPTS += LD="$(LD)" LDFLAGS="$(LDFLAGS)"


should already be covered by CONFIGURE_COMMON

I can put it in CONFIGURE_COMMON but it isn't there right now.

you're adding it there below

I'm an idiot.

tkelman · 2017-05-12T20:49:53Z

needs to be tested that cmake doesn't have any issues in cross-compilation with the new flag

ararslan · 2017-05-12T20:51:02Z

needs to be tested that cmake doesn't have any issues in cross-compilation with the new flag

How does one do that? Can you run it through the buildbots or something?

staticfloat · 2017-05-12T21:12:19Z

How does one do that? Can you run it through the buildbots or something?

If you just mention @jlbuild on a line on of its own in a comment, it'll take the state of this current PR and run it through the buildbots. It's got a few other interesting features such as running on a subset of the buildbots (!filter=foo,bar) or building completely cleanly (!nuke). Here's an example:

@jlbuild

jlbuild · 2017-05-12T21:12:21Z

Status of `2bab4cf` builds:

Builder Name	Build	Download
linux32	COMPLETE	Download
linux64	COMPLETE	Download
linuxaarch64	COMPLETE	Download
linuxarmv7l	ERRORED	N/A
linuxppc64le	COMPLETE	Download
osx64	COMPLETE	Download
win32	PENDING	N/A
win64	CANCELED	N/A

tkelman · 2017-05-12T21:26:13Z

jlbuild isn't running windows yet, and we don't have linux-to-windows cross compile build bots. see README.windows.md, but if you have cross compilers installed then it should just be a matter of setting XC_HOST=x86_64-w64-mingw32

tkelman · 2017-05-12T21:27:25Z

deps/llvm.mk

@@ -73,6 +73,7 @@ LLVM_FLAGS += --disable-bindings --disable-docs --disable-libedit --disable-term
 # LLVM has weird install prefixes (see llvm-$(LLVM_VER)/build_$(LLVM_BUILDTYPE)/Makefile.config for the full list)
 # We map them here to the "normal" ones, which means just prefixing "PROJ_" to the variable name.
 LLVM_MFLAGS := PROJ_libdir=$(build_libdir) PROJ_bindir=$(build_depsbindir) PROJ_includedir=$(build_includedir)
+LLVM_MFLAGS += LD="$(LD)"


shouldn't this be redundant with setting it via cmake?

Probably, but I don't actually know, so I added it here just in case. Not sure how exactly to figure out whether it's in fact redundant.

looks like we don't actually use LLVM_MFLAGS when building with cmake, so this is only relevant for llvm 3.7 or earlier

Oh, okay. So is this fine as is, then? There's also the LLVM_USE_CMAKE flag (or something like that) but I don't know when one would actually want to use it.

usually to force old llvm versions to use cmake instead of defaulting to configure, which may have been useful once in a while

ararslan · 2017-05-13T00:07:56Z

if you have cross compilers installed

I don't :/

The ARMv7 failure is weird--could that be related?

staticfloat · 2017-05-13T00:09:24Z

ARMv7 is broken at the moment, we found a subtle bug in Julia that Yichao is patching here

ararslan · 2017-05-14T16:09:11Z

Would someone familiar with cross-compilation be willing to test this?

tkelman · 2017-05-14T16:25:35Z

I did a bit of testing, I think it should be okay. Was hitting some unrelated errors, where googling led me to a comment in one of my own dockerfiles from many months ago.

ararslan · 2017-05-15T17:30:45Z

Great, thanks for testing. Is this good to go then?

ararslan added the building Build system, or building Julia or its dependencies label May 9, 2017

ararslan requested review from staticfloat and tkelman May 9, 2017 23:01

staticfloat reviewed May 9, 2017

View reviewed changes

ararslan force-pushed the aa/respect-linker branch from 645164c to 2bab4cf Compare May 9, 2017 23:09

tkelman reviewed May 12, 2017

View reviewed changes

ararslan force-pushed the aa/respect-linker branch from 2bab4cf to 0ad59d3 Compare May 12, 2017 21:12

tkelman reviewed May 12, 2017

View reviewed changes

Pass LD and LDFLAGS to more dependencies

1e90fde

ararslan force-pushed the aa/respect-linker branch from 0ad59d3 to 1e90fde Compare May 18, 2017 04:39

ararslan merged commit 259daca into master May 18, 2017

ararslan deleted the aa/respect-linker branch May 18, 2017 05:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass LD and LDFLAGS to more dependencies #21764

Pass LD and LDFLAGS to more dependencies #21764

ararslan commented May 9, 2017

staticfloat May 9, 2017

ararslan May 9, 2017

tkelman May 9, 2017

tkelman May 13, 2017

tkelman commented May 9, 2017

ararslan commented May 9, 2017

tkelman commented May 9, 2017

ararslan commented May 10, 2017

tkelman commented May 10, 2017

staticfloat commented May 10, 2017

staticfloat commented May 10, 2017

ararslan commented May 12, 2017

tkelman May 12, 2017

ararslan May 12, 2017

tkelman May 12, 2017

ararslan May 12, 2017

tkelman commented May 12, 2017

ararslan commented May 12, 2017

staticfloat commented May 12, 2017

jlbuild commented May 12, 2017 •

edited

Loading

tkelman commented May 12, 2017

tkelman May 12, 2017

ararslan May 12, 2017

tkelman May 12, 2017

ararslan May 13, 2017

tkelman May 13, 2017

ararslan commented May 13, 2017

staticfloat commented May 13, 2017

ararslan commented May 14, 2017

tkelman commented May 14, 2017

ararslan commented May 15, 2017

Pass LD and LDFLAGS to more dependencies #21764

Pass LD and LDFLAGS to more dependencies #21764

Conversation

ararslan commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented May 9, 2017

ararslan commented May 9, 2017

tkelman commented May 9, 2017

ararslan commented May 10, 2017

tkelman commented May 10, 2017

staticfloat commented May 10, 2017

staticfloat commented May 10, 2017

ararslan commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented May 12, 2017

ararslan commented May 12, 2017

staticfloat commented May 12, 2017

jlbuild commented May 12, 2017 • edited Loading

Status of 2bab4cf builds:

tkelman commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented May 13, 2017

staticfloat commented May 13, 2017

ararslan commented May 14, 2017

tkelman commented May 14, 2017

ararslan commented May 15, 2017

jlbuild commented May 12, 2017 •

edited

Loading

Status of `2bab4cf` builds: