Skip to content

CMake: Build both static and shared sets of stdlibs#1960

Merged
kinke merged 2 commits intoldc-developers:masterfrom
kinke:sharedCMake
Mar 29, 2017
Merged

CMake: Build both static and shared sets of stdlibs#1960
kinke merged 2 commits intoldc-developers:masterfrom
kinke:sharedCMake

Conversation

@kinke
Copy link
Member

@kinke kinke commented Jan 8, 2017

Via -DBUILD_SHARED_LIBS=BOTH, addressing the build part of #1282.

@kinke
Copy link
Member Author

kinke commented Jan 8, 2017

With -DBUILD_SHARED_LIBS=BOTH -DMULTILIB=ON on Linux:

> ls lib lib32
lib:
libdruntime-ldc.a                       libdruntime-ldc-shared.so.2.0.71  libphobos2-ldc-debug-shared.so
libdruntime-ldc-debug.a                 libdruntime-ldc-shared.so.71      libphobos2-ldc-debug-shared.so.2.0.71
libdruntime-ldc-debug-shared.so         libldc-profile-rt.a               libphobos2-ldc-debug-shared.so.71
libdruntime-ldc-debug-shared.so.2.0.71  libldc-profile-rt-shared.a        libphobos2-ldc-shared.so
libdruntime-ldc-debug-shared.so.71      libphobos2-ldc.a                  libphobos2-ldc-shared.so.2.0.71
libdruntime-ldc-shared.so               libphobos2-ldc-debug.a            libphobos2-ldc-shared.so.71

lib32:
libdruntime-ldc.a                       libdruntime-ldc-shared.so.2.0.71  libphobos2-ldc-debug-shared.so
libdruntime-ldc-debug.a                 libdruntime-ldc-shared.so.71      libphobos2-ldc-debug-shared.so.2.0.71
libdruntime-ldc-debug-shared.so         libldc-profile-rt.a               libphobos2-ldc-debug-shared.so.71
libdruntime-ldc-debug-shared.so.2.0.71  libldc-profile-rt-shared.a        libphobos2-ldc-shared.so
libdruntime-ldc-debug-shared.so.71      libphobos2-ldc.a                  libphobos2-ldc-shared.so.2.0.71
libdruntime-ldc-shared.so               libphobos2-ldc-debug.a            libphobos2-ldc-shared.so.71

@kinke
Copy link
Member Author

kinke commented Jan 8, 2017

This needs some testing on OSX wrt. merging the 32/64-bit libs.

@kinke
Copy link
Member Author

kinke commented Jan 8, 2017

As to the other aspect of #1282, the selection of the stdlib to be linked against, I'm still favoring #1735 - hardcoded lib suffix [-debug][-shared] defined via new -link-sharedlib (use DLL) besides existing -link-debuglib (use debug instead of release), configurable lib stems via existing -defaultlib=myDruntime,myPhobos, and deprecated (and ignored) -debuglib=....

@kinke
Copy link
Member Author

kinke commented Jan 22, 2017

Pinging @JohanEngelen and @klickverbot - someone please test make install after CMake -DBUILD_SHARED_LIBS=BOTH -DMULTILIB=ON on OSX, in order to test merging the x86 and x86_64 libs into these 'fat' libs (that's the open WIP aspect about this PR).

@dnadlinger
Copy link
Member

Will do. We should also make it the default.

@kinke
Copy link
Member Author

kinke commented Jan 22, 2017

Setting it to default will double the tests (at least the unittests), so we'll need to slightly tweak the CI scripts.

If it wasn't for -relocation-model=pic, we wouldn't have to compile the Phobos modules (and tests!) twice. The druntime and profile-rt modules get an additional -d-version=Shared, so they need to be recompiled (but they are very quick in comparison anyway).

@dnadlinger
Copy link
Member

With -DBUILD_SHARED_LIBS=BOTH -DMULTILIB=ON:

CMake Error at runtime/CMakeLists.txt:805 (add_test):
  Error evaluating generator expression:

    $<TARGET_FILE:druntime-ldc-shared>

  Target "druntime-ldc-shared" is not an executable or library.

Will have a quick look.

@dnadlinger
Copy link
Member

Partial patch for OS X:

--- a/runtime/CMakeLists.txt
+++ b/runtime/CMakeLists.txt
@@ -405,6 +405,7 @@ macro(build_runtime d_flags c_flags ld_flags lib_suffix path_suffix is_shared ou
     endif()
     compile_druntime("${druntime_d_flags}" "${lib_suffix}" "${path_suffix}" druntime_o druntime_bc)
 
+    get_target_suffix("${lib_suffix}" "${path_suffix}" target_suffix)
     add_library(druntime-ldc${target_suffix} ${library_type}
         ${druntime_o} ${CORE_C} ${DCRT_C} ${DCRT_ASM})
     set_target_properties(
@@ -569,12 +570,8 @@ if(MULTILIB)
         build_runtime_variants("-m${MULTILIB_SUFFIX}" "-m${MULTILIB_SUFFIX} ${RT_CFLAGS}" "-m${MULTILIB_SUFFIX} ${LD_FLAGS}" "${MULTILIB_SUFFIX}" multitargets)
 
         foreach(targetname ${hosttargets})
-            set(lib_path "$<TARGET_FILE:${targetname}>")
-            set(lib_extension "")
-            get_filename_component(lib_extension ${lib_path} EXT)
-
             string(REPLACE "_${hostsuffix}" "" t ${targetname})
-            set(lib_filename lib${t}${lib_extension})
+            set(lib_filename lib${t}${SHARED_LIB_SUFFIX}${CMAKE_SHARED_LIBRARY_SUFFIX})
 
             add_custom_command(
                 OUTPUT ${CMAKE_BINARY_DIR}/lib${LIB_SUFFIX}/${lib_filename}
@@ -790,7 +787,13 @@ endif()
 # Add the standalone druntime tests.
 # TODO: Add test/excetions and test/init_fini.
 if(NOT ${BUILD_SHARED_LIBS} STREQUAL "OFF")
-    set(druntime_path "$<TARGET_FILE:druntime-ldc${SHARED_LIB_SUFFIX}>")
+    if(MULTILIB AND APPLE)
+        # KLUDGE: The library target is a custom command for multilib builds (lipo),
+        # so cannot use TARGET_FILE directly. Should stash away that name instead.
+        set(druntime_path "${CMAKE_BINARY_DIR}/lib${LIB_SUFFIX}/libdruntime-ldc.dylib")
+    else()
+        set(druntime_path "$<TARGET_FILE:druntime-ldc${SHARED_LIB_SUFFIX}>")
+    endif()
     set(outdir ${PROJECT_BINARY_DIR}/druntime-test-shared)
 
     add_test(NAME clean-druntime-test-shared

Now it fails because profile-rt is always built as a static library.

@dnadlinger
Copy link
Member

(Just not supporting shared multilib builds on OS X at all might actually be a reasonable option.)

@kinke
Copy link
Member Author

kinke commented Feb 9, 2017

Thanks - what's the point of these OSX 'fat' libs? Are they just a fancy way of cutting the number of lib files in half or does Apple enforce that format?

Wrt. profile-rt, static libs were a deliberate choice for -link-sharedlib too. Should they be shared instead or should -d-version=Shared NOT be defined when compiling static libprofile-rt-shared.a for binaries linked to shared druntime/Phobos?

@kinke
Copy link
Member Author

kinke commented Feb 20, 2017

Still awaiting an answer wrt. any advantages of OSX fat libs... ;)

If it wasn't for -relocation-model=pic, we wouldn't have to compile the Phobos modules (and tests!) twice.

If we emit PIC by default on Linux anyway, we should exploit that and build the Phobos modules only once, greatly speeding up the build process. Making it a Linux special case will clutter the CMake m(adn)ess even more; would emitting PIC by default on OSX be an option too? oh, just seen that we already do that for OSX too. And LLVM defaults to PIC on Win64 as well.

@dnadlinger
Copy link
Member

Still awaiting an answer wrt. any advantages of OSX fat libs... ;)

It's the way to do multilib on OS X – there exists no infrastructure for handling different lib{32, 64} directories, etc. like on Linux.

Wrt. profile-rt, static libs were a deliberate choice for -link-sharedlib too.

Having a static libprofile-rt.a is probably a sensible choice – but it means special-casing the OS X build system.

Should they be shared instead or should -d-version=Shared NOT be defined when compiling static libprofile-rt-shared.a for binaries linked to shared druntime/Phobos?

-d-version=Shared only affects the druntime internals. libprofile-rt is a standalone C/C++ library (apart from the small ldc.profile client program interface).

@kinke kinke force-pushed the sharedCMake branch 4 times, most recently from 628382c to 25619f0 Compare February 27, 2017 00:07
@kinke
Copy link
Member Author

kinke commented Feb 27, 2017

Building shared druntime/Phobos libs (incl. unittest libs and running those tests) in addition to the static libs and tests now only requires a few additional minutes on Travis.

@kinke
Copy link
Member Author

kinke commented Feb 28, 2017

From the Travis LLVM 3.8 MULTILIB=ON BUILD_SHARED_LIBS=BOTH overkill job log:

99% tests passed, 1 tests failed out of 3090

Total Test time (real) = 224.19 sec

The following tests FAILED:
	2859 - rt.util.typeinfo-debug-shared_32 (Failed)
Errors while running CTest


The command "ctest -j3 --output-on-failure -E "dmd-testsuite|lit-tests"" exited with 8.
store build cache

changes detected, packing new archive
.


The job exceeded the maximum time limit for jobs, and has been terminated.

So it only just just made it under 50 mins. :D More than 3,000 unittested druntime/Phobos modules (8x druntime/Phobos: release/debug, shared/static, x86/x86_64)...

@kinke
Copy link
Member Author

kinke commented Feb 28, 2017

Travis:

Linking C shared library ../lib64/libdruntime-ldc-unittest-debug-shared.so
CMake Error: cmake_symlink_library: System Error: File exists
make[3]: *** [lib64/libdruntime-ldc-unittest-debug-shared.so.2.0.72] Error 1

Seems to happen if the job previously built shared libs too (1st run works, 2nd doesn't, 3rd does...). Any idea why the LDC build directory or at least the links seem to be cached across Travis runs?!

@kinke
Copy link
Member Author

kinke commented Feb 28, 2017

@klickverbot: Please test the merged libs on OSX again.

@dnadlinger
Copy link
Member

dnadlinger commented Mar 1, 2017

@kinke:

$ cmake -G Ninja -DLLVM_CONFIG=/opt/llvm-3.9-release/bin/llvm-config -DBUILD_SHARED_LIBS=BOTH -DMULTILIB=ON ~/Build/Source/ldc
-- Found host D compiler /usr/local/bin/ldmd2, with default flags ''
-- Host D compiler version: LDC - the LLVM D compiler (339d87)
-- LDC version identifier: 1.2.0git-5679ad4
-- Building LDC with PGO support
-- Host D compiler linker program: /usr/bin/gcc
-- Host D compiler linker flags: -L/usr/local/lib;-lphobos2-ldc;-ldruntime-ldc;-ldl;-lpthread;-lm;-m64
-- Also installing LTO binary: /opt/llvm-3.9-release/lib/libLTO.dylib
-- Using path for Intrinsics.td: /opt/llvm-3.9-release/include
CMake Error at runtime/CMakeLists.txt:563 (add_custom_command):
  add_custom_command called with OUTPUT containing a "<".  This character is
  not allowed.


CMake Error at runtime/CMakeLists.txt:563 (add_custom_command):
  add_custom_command called with OUTPUT containing a "<".  This character is
  not allowed.


CMake Error at runtime/CMakeLists.txt:563 (add_custom_command):
  add_custom_command called with OUTPUT containing a "<".  This character is
  not allowed.


CMake Error at runtime/CMakeLists.txt:563 (add_custom_command):
  add_custom_command called with OUTPUT containing a "<".  This character is
  not allowed.


CMake Error at runtime/CMakeLists.txt:563 (add_custom_command):
  add_custom_command called with OUTPUT containing a "<".  This character is
  not allowed.


CMake Error at runtime/CMakeLists.txt:563 (add_custom_command):
  add_custom_command called with OUTPUT containing a "<".  This character is
  not allowed.


CMake Error at runtime/CMakeLists.txt:563 (add_custom_command):
  add_custom_command called with OUTPUT containing a "<".  This character is
  not allowed.


CMake Error at runtime/CMakeLists.txt:563 (add_custom_command):
  add_custom_command called with OUTPUT containing a "<".  This character is
  not allowed.


CMake Error at runtime/CMakeLists.txt:563 (add_custom_command):
  add_custom_command called with OUTPUT containing a "<".  This character is
  not allowed.

This is a snag I also ran into (hence the hardcoded bit in the partial patch) – as the name suggests, CMake generator expressions ($<…>) are evaluated by the build system generator, and can't directly be used in other contexts. If you can't figure out a solution, I would be fine with just disallowing the shared/multilib combination for macOS for now (as long as there is a clear error message for it).

@kinke
Copy link
Member Author

kinke commented Mar 1, 2017

I had another solution - all 9 potential libs (per architecture) hardcoded, until I realized this friggin $<TARGET_LINKER_FILE:...> ought to work. Damn CMake, I hate it (but sadly I also don't know a better alternative).

The main reason for these generator expressions is that the target druntime-ldc may produce a libdruntime-ldc.a (BUILD_SHARED_LIBS != ON) or libdruntime-ldc.so (BUILD_SHARED_LIBS == ON => only generating shared libs). And the reason for that is that adding SHARED_LIB_SUFFIX (-shared) to all shared-lib targets and files for BUILD_SHARED_LIBS == ON too would complicate generating the default config file and misc. command lines in our CI scripts. Might be worth checking out the required adaptations though, they may be less tedious than further CMake hacking.

@dnadlinger
Copy link
Member

You are using target_name now on CMakeLists.txt:576 which doesn't refer to anything (or rather to the previous declaration from the gccbuiltins code.

@kinke
Copy link
Member Author

kinke commented Mar 3, 2017

Well now it looks like the merged shared libs don't work:

ld: warning: ignoring file /Users/travis/build/ldc-developers/ldc/lib/libdruntime-ldc-unittest-shared.dylib, file was built for x86_64 which is not the architecture being linked (i386): /Users/travis/build/ldc-developers/ldc/lib/libdruntime-ldc-unittest-shared.dylib.

I won't pursue OSX further. I don't have it, Travis takes hours to finally process the OSX jobs, so someone working on OSX please fix the last issues, I'm done with this.

Edit: Actually, the reason is that the unittest libs aren't merged, unlike the regular non-unittest .dylibs.

@kinke
Copy link
Member Author

kinke commented Mar 3, 2017

the unittest libs aren't merged

And never have been, so the unittests with MULTILIB=ON on OSX most likely never worked. Not tested by CI, so I won't put any more effort into it.

@kinke kinke changed the title [WIP] CMake: Enable building both static and shared sets of stdlibs CMake: Enable building both static and shared sets of stdlibs Mar 3, 2017
set(D_LIBRARY_TYPE SHARED)
else()
set(D_LIBRARY_TYPE STATIC)
# Only use the `-shared` lib suffix if static libs are generated too
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to always use -shared for uniformity? (e.g. if somebody explicitly needs to link against druntime/Phobos in their custom build system, etc.)

Copy link
Member Author

@kinke kinke Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding SHARED_LIB_SUFFIX (-shared) to all shared-lib targets and files for BUILD_SHARED_LIBS == ON too would complicate generating the default config file and misc. command lines in our CI scripts.

It'd also be a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest always adding -shared as soon as we have -link-sharedlib or something similar.

It's -link-sharedlib vs. -defaultlib=druntime-ldc-shared,phobos2-ldc-shared -debuglib=druntime-ldc-debug-shared,phobos2-ldc-debug-shared.

@kinke kinke changed the title CMake: Enable building both static and shared sets of stdlibs CMake: Build both static and shared sets of stdlibs Mar 3, 2017
@kinke
Copy link
Member Author

kinke commented Mar 3, 2017

As mentioned somewhere else, I suggest putting this, something like #1735 and #2016 into a single LDC release (1.3). Rationale being that they all affect the build system, and distro maintainers etc. should only have to adapt their scripts once.

@kinke kinke mentioned this pull request Mar 6, 2017
@kinke kinke mentioned this pull request Mar 18, 2017
kinke added 2 commits March 19, 2017 00:05
Via `-DBUILD_SHARED_LIBS=BOTH`.

Compile the Phobos modules only once for static+shared libs.

Build a single ldc-profile-rt.a (per architecture); `-d-version=Shared`
was just copy-pasted and not needed, so no need for an extra
ldc-profile-rt-shared.a.
@kinke kinke added this to the 1.3.0 milestone Mar 25, 2017
@kinke
Copy link
Member Author

kinke commented Mar 25, 2017

Any concerns? Otherwise I'll merge it in a couple of days. This should have been tested quite extensively by intermediate commits (with -DBUILD_SHARED_LIBS=BOTH -DMULTILIB=ON, on both Linux and OSX, with installing LDC etc.).

@kinke kinke merged commit 30bc6e3 into ldc-developers:master Mar 29, 2017
@kinke kinke deleted the sharedCMake branch March 29, 2017 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants