Installation Folder & rpath urls #103

gabyx · 2018-01-21T19:47:41Z

the same as in #101 ,

…make it works

gabyx · 2018-01-21T21:42:40Z

This is now pretty fine -> executable work in build bin folder (on macos)
After installation the binary no longer work, because the rpaths have not been adapted. Cmake should actually do something when installing (see https://cmake.org/Wiki/CMake_RPATH_handling)

the output of otool -l /usr/local/opt/rttr/bin/bench_variant | grep LC_RPATH -A2

          cmd LC_RPATH
      cmdsize 40
         path /usr/local/opt/llvm/lib (offset 12)

where as the binary in the build folder works and has

         cmd LC_RPATH
      cmdsize 40
         path /usr/local/opt/llvm/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 56
         path /Users/gabrielnuetzi/Desktop/rttrBuild/lib (offset 12)

this is the reason it work in the build folder, I dont understand why cmake has not correctly adapted the RPATH after installing, there is a misconfiguration I think...

acki-m · 2018-01-21T22:20:19Z

CMake/utility.cmake

@@ -165,7 +165,7 @@ function(loadFolder FOLDER _HEADER_FILES _SOURCE_FILES)
    getNameOfDir(CMAKE_CURRENT_SOURCE_DIR DIRNAME)
    if (${shouldInstall})
      if (NOT ${FULL_HEADER_PATH} MATCHES ".*_p.h$") # we don't want to install header files which are marked as private
-        install(FILES ${FULL_HEADER_PATH} DESTINATION "include/${DIRNAME}/${REL_PATH}" PERMISSIONS OWNER_READ)


why is this removed? I want that the installed header files should not be modified by accident.

wrong merge, I will correct that

…es...

gabyx · 2018-01-21T23:53:07Z

thanks, its a mistake!

coveralls · 2018-01-22T01:31:18Z

Coverage remained the same at 90.131% when pulling 764c0c7 on gabyx:fix-cmake-rpath-macos into 4a0272e on rttrorg:master.

Warchant · 2018-01-22T09:42:21Z

CMakeLists.txt

@@ -31,7 +31,7 @@
 # the build configuration options are initialized.                                 #
 ####################################################################################

-cmake_minimum_required (VERSION 2.8.12)
+cmake_minimum_required (VERSION 3.10)


3.10 version is too strict, IMO 3.0 is enough, unless you use something specific.

thanks!
ok, cmake version 3.0 is fine for me, I rather work with the newest, since there is better behavior :-)

and it is not a problem for the user to have higher version

Warchant · 2018-01-22T12:06:55Z

$ mkdir build
$ cd build
$ cmake .. 
$ make

fails with

[ 99%] Linking CXX executable ../../bin/unit_tests
[ 99%] Built target unit_tests
Scanning dependencies of target run_tests
[100%] Running unit_tests
../../bin/unit_tests: error while loading shared libraries: librttr_core.so.0.9.6: cannot open shared object file: No such file or directory
src/unit_tests/CMakeFiles/run_tests.dir/build.make:57: recipe for target 'src/unit_tests/CMakeFiles/run_tests' failed
make[2]: *** [src/unit_tests/CMakeFiles/run_tests] Error 127
CMakeFiles/Makefile2:206: recipe for target 'src/unit_tests/CMakeFiles/run_tests.dir/all' failed
make[1]: *** [src/unit_tests/CMakeFiles/run_tests.dir/all] Error 2
Makefile:149: recipe for target 'all' failed
make: *** [all] Error 2

Linux Ubuntu 16.04

gabyx · 2018-01-22T13:51:47Z

@Warchant : Could you extract some information in the unit_test binaries (I could not build them catch is too old for gcc 7 or clang 7).

I mean what is the RPATH the unit tests executable is searching for the library,

https://cmake.org/Wiki/CMake_RPATH_handling
objdump -x executable | grep RPATH

I think we miss some configuration for INSTALL_RPATH here:

rttr/src/unit_tests/CMakeLists.txt

Line 46 in 47d576c

set_target_properties(unit_tests PROPERTIES DEBUG_POSTFIX _d)

but that line I added to the other exes, is for installation only, I think I have to check on my linux machine

INSTALL_RPATH "${RTTR_EXECUTABLE_INSTALL_RPATH}"

gabyx · 2018-01-22T21:18:46Z

Building with gcc4.9 (macOs).... lets see whats the problem

gabyx · 2018-01-22T21:58:49Z

ok found the error, I think I mistakenly misscorrected the beahvior for rpath...

gabyx · 2018-01-22T22:55:57Z

lets look on the tests, the unit tests build with gcc4.9 on mac, libraries are loaded, but no executable does run (unit_tests and benchmarks),
: weird error:

bench_variant(24239,0x7fff74ebc000) malloc: *** error for object 0x106ecbea0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
clock resolution: Abort trap: 6

@acki-m: Any idea? Supposed wrong linking, ...?
with clang7.0 and all warnings off I builded everythin except the unit_tests since catch is too old?
are you improving on this? meaning, installing a newer version!? that would be great!
So we can make some progress with some really more modern compilers instead of the ones supported, I think they are way behind...

gabyx · 2018-01-23T18:35:52Z

seems to compile now, apple travis is missing, they have problems I think...

gabyx · 2018-01-23T22:37:11Z

OK: only https://travis-ci.org/rttrorg/rttr/jobs/332456069 still fails due to some installation problems , probably re-run it again....

acki-m · 2018-01-24T09:21:16Z

What are you fixing here? What is the problem on MacOSX?

gabyx · 2018-01-24T10:27:04Z

Changed the PR title: I made more consistent cmake code, for linux and mac, windows, and set the settings for the rpath urls in the binaries.
as well as incorporated the changes of @Warchant (some of them)
Default prefix when configuring is still ./install, these are some really valuable changes I think.

Newer Cmake
Made rpath settings in binaries consistent in Linux/Mac (Windows not affected since that thing does not exist in Win) (newer cmake has some new policy for macos, thats why it did not work)
Better Install directories (default is ./install)

These fixes are also incorporated for the PR #100 (which is on good way I think, since, clang 7.0 compiles fine with all warning on, except for benchmarks and unit_tests there I have disabled some minor ones... (see the PR)

gabyx · 2018-01-24T18:17:09Z

Can we merge this?

acki-m · 2018-01-24T19:21:31Z

I checked it out and compiled it with MSVC. It works so far, however.
I am unsure about:

higher CMake version 3.5
the switching of: -stdlib=libstdc++ -static -lstdc++
DESTINATION ${CMAKE_INSTALL_DATADIR}/rttr/cmake What location is this?
how to you copy the readme and the license file now?

PS: I would merge this first, because this fixes the path handling

gabyx · 2018-01-24T19:26:37Z

that would be great!,

${CMAKE_INSTALL_DATADIR}= /usr/local/share
on windows its set to the CMAKE_INSTALL_PREFIX
higher CMake version 3.5 -> set it back to 3.0 in clang 7.0 support - individual warning levels #100
readme file back added in clang 7.0 support - individual warning levels #100 (was a merge mistake I think)

acki-m · 2018-01-24T19:38:51Z

Damn't it I checked out master from your fork ^^
That's why everything worked. Have to do it again.

gabyx · 2018-01-24T19:43:23Z

Oh pitty, ehm -> let me know if something is not working, so we can fix it =)

gabyx · 2018-01-24T19:43:37Z

At least appveyor succeded, the others are a bit stuck

acki-m · 2018-01-24T20:00:04Z

CMake/config.cmake

+  set(CMAKE_INSTALL_DATADIR "${CMAKE_INSTALL_PREFIX}")
+endif()
+
+set(CMAKE_DEBUG_POSTFIX CACHE STRING "_d")


for unkown reason the postfix will not be applied to rttr_core

The postfix is necessary, otherwise we cannot copy the debug and release libraries, into one folder.

for sure! but why is it not working? the CMAKE_DEBUG_POSTFIX is set...?

acki-m · 2018-01-24T20:00:51Z

CMake/config.cmake

             COMPONENT Devel)
-
-    install(FILES "${LICENSE_FILE}" "${README_FILE}"


license files are missing

acki-m · 2018-01-24T20:02:12Z

CMake/config.cmake

@@ -134,10 +161,6 @@ write_basic_package_version_file(

 if (BUILD_INSTALLER)
    install(FILES "${CMAKE_CURRENT_BINARY_DIR}/CMake/rttr-config-version.cmake"
-             DESTINATION cmake
+             DESTINATION ${CMAKE_INSTALL_DATADIR}/rttr/cmake


this looks now strange on windows:

The folder should be named cmake. I think otherwise cmake will not find the script, when pointing to the root directory

the lib folder should be empty? isnt it? the libraries should all go into the bin folder...?

may be we could symlink on windows into the lib folder, and install all everythin properly in the lib folder?, why is the lib folder even constructed?

The lib folder has to be there, there are the *.lib files placed.
necessary to link the libraries on windows

ah, where is this path hardcoded into cmake? i didnt see that one? maybe I missed something?

acki-m · 2018-01-24T20:02:26Z

CMakeLists.txt

@@ -31,7 +31,7 @@
 # the build configuration options are initialized.                                 #
 ####################################################################################

-cmake_minimum_required (VERSION 2.8.12)
+cmake_minimum_required (VERSION 3.5)


change to 3.0

gabyx · 2018-01-24T20:14:06Z

fixed, strange with the debug post fix? does it work with a reconfigure?

…rttr-config.cmake

…RTTR_ variables

acki-m · 2018-01-24T20:44:58Z

The lib folder should stay!
That's how it works under windows, that is common behavior to put the libs under lib
Here how it is done with Qt:

PS: the debug DLL has still no _d postfix
Do you have not a virtual machine with windows for testing?

acki-m · 2018-01-24T20:47:37Z

This will fix it:

    set_target_properties(rttr_core PROPERTIES
                          VERSION ${RTTR_VERSION} SOVERSION ${RTTR_VERSION}
                          DEBUG_POSTFIX _d
                          EXPORT_NAME Core)

gabyx · 2018-01-24T20:53:26Z

think I found the error, fixing it

acki-m · 2018-01-24T20:55:18Z

.gitignore

@@ -0,0 +1,2 @@
+.vscode
+build


add new line

acki-m · 2018-01-24T20:55:57Z

CMake/config.cmake

 endif()

-set(CMAKE_DEBUG_POSTFIX "_d")
+set(CMAKE_DEBUG_POSTFIX CACHE STRING "_d")


I think this can be removed.
You have to add the DEBUG_POSTFIX _d to all target properties

…once and forall

acki-m · 2018-01-24T22:04:16Z

It seems to work now, I wait for the MSVC builds to finish, then I will merge
PS: Why did you changed the -stdlib=libstdc++ -static -lstdc++

gabyx · 2018-01-24T22:05:50Z

jup, for me to, on mac os, installation folder looks clean. tests passed...

gabyx · 2018-01-24T22:06:45Z

Oh, it dont know why I changed, -stdlib=libstdc++ -static -lstdc++
@Warchant I think commented on this beeing more standart? maybe we should check again... do you know?

acki-m · 2018-01-25T08:50:59Z

@gabyx
I want to squash your 21 commits into one for merge, can you give me a shot summary, what you have done here.

gabyx · 2018-01-25T13:44:46Z

Jeah, thats a good thing to do,
Shot summary:

installation folder consistency Linux/Mac/Windows solved
RPath issues solved for Linux/Mac
cmake version upgraded to 3.0
I think thats it.

sorry, I was a bit profusely with comitting

Thanks for your time!

gabyx added 4 commits January 21, 2018 19:14

fix rpath on mac os, such that when linking with this library using c…

ef43f83

…make it works

remove uneeded settings to fix the problem

839f13f

merged the good parts from warchant

60951cf

merged the good parts from warchant, restored some default behaviour

e1999ea

gabyx mentioned this pull request Jan 21, 2018

Fix installation on Linux/Mac machines #101

Closed

6 tasks

acki-m reviewed Jan 21, 2018

View reviewed changes

gabyx added 2 commits January 22, 2018 00:21

rpath and install_names fixed, on windows libraries go next to binari…

ee4f284

…es...

fix, typo

c181f4e

Warchant reviewed Jan 22, 2018

View reviewed changes

gabyx added 3 commits January 22, 2018 23:13

corrected rpath settings

4579a90

permissions fix

64105cd

default prefix fix

a6661d7

travis cmake update

e3c1dc2

gabyx mentioned this pull request Jan 23, 2018

correct install path for libraries -> lib instead of bin #102

Closed

gabyx added 4 commits January 23, 2018 20:22

Merge remote-tracking branch 'upstream/master'

5b65744

Merge branch 'master' into fix-cmake-rpath-macos

c4990f8

cmake fixes, rpath for benchmarks

f975fff

cmake fixes, typo

b78ef29

gabyx changed the title ~~Fix cmake rpath on macos~~ Fix cmake rpath settings Jan 24, 2018

gabyx changed the title ~~Fix cmake rpath settings~~ Installation Folder & rpath urls Jan 24, 2018

gabyx mentioned this pull request Jan 24, 2018

clang 7.0 support - individual warning levels #100

Merged

acki-m requested changes Jan 24, 2018

View reviewed changes

Review changes

87d0784

gabyx added 2 commits January 24, 2018 21:25

Review changes, everything into bin directory on windows..., fix for …

f45e587

…rttr-config.cmake

renaming, better read out GNUInstallDirs, set them, and use only the …

2049bc8

…RTTR_ variables

Debug postfix

14781a0

acki-m requested changes Jan 24, 2018

View reviewed changes

gabyx added 3 commits January 24, 2018 22:07

Installation dir correction, make it explicit, set it in the config, …

fc95f8e

…once and forall

Review changes

d34f713

typos

764c0c7

acki-m merged commit 29e6ff9 into rttrorg:master Jan 25, 2018

gabyx deleted the fix-cmake-rpath-macos branch February 16, 2018 18:34

		COMPONENT Devel)

		install(FILES "${LICENSE_FILE}" "${README_FILE}"

Installation Folder & rpath urls #103

Installation Folder & rpath urls #103

Conversation

gabyx commented Jan 21, 2018

gabyx commented Jan 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabyx commented Jan 21, 2018

coveralls commented Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Warchant commented Jan 22, 2018 • edited Loading

gabyx commented Jan 22, 2018 • edited Loading

gabyx commented Jan 22, 2018

gabyx commented Jan 22, 2018

gabyx commented Jan 22, 2018

gabyx commented Jan 23, 2018

gabyx commented Jan 23, 2018

acki-m commented Jan 24, 2018

gabyx commented Jan 24, 2018

gabyx commented Jan 24, 2018

acki-m commented Jan 24, 2018

gabyx commented Jan 24, 2018 • edited Loading

acki-m commented Jan 24, 2018

gabyx commented Jan 24, 2018

gabyx commented Jan 24, 2018 • edited Loading

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

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

gabyx commented Jan 24, 2018 via email • edited Loading

acki-m commented Jan 24, 2018

acki-m commented Jan 24, 2018

gabyx commented Jan 24, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acki-m commented Jan 24, 2018

gabyx commented Jan 24, 2018

gabyx commented Jan 24, 2018

acki-m commented Jan 25, 2018

gabyx commented Jan 25, 2018 • edited Loading

coveralls commented Jan 22, 2018 •

edited

Loading

Warchant commented Jan 22, 2018 •

edited

Loading

gabyx commented Jan 22, 2018 •

edited

Loading

gabyx commented Jan 24, 2018 •

edited

Loading

gabyx commented Jan 24, 2018 •

edited

Loading

gabyx commented Jan 24, 2018 via email •

edited

Loading

gabyx commented Jan 25, 2018 •

edited

Loading