Merge lp:~jpakkane/unity-scopes-api/randomstuff into lp:unity-scopes-api

Proposed by Jussi Pakkanen
Status: Merged
Approved by: Michi Henning
Approved revision: 5
Merged at revision: 5
Proposed branch: lp:~jpakkane/unity-scopes-api/randomstuff
Merge into: lp:unity-scopes-api
Diff against target: 116 lines (+20/-14)
7 files modified
CMakeLists.txt (+2/-3)
test/CMakeLists.txt (+4/-0)
test/gtest/unity/api/scopes/ScopeBase/CMakeLists.txt (+0/-3)
test/gtest/unity/api/scopes/ScopeBase/ScopeBase_test.cpp (+2/-2)
test/gtest/unity/api/scopes/internal/DynamicLoader/CMakeLists.txt (+0/-3)
test/gtest/unity/api/scopes/internal/DynamicLoader/DynamicLoader_test.cpp (+4/-3)
test/scope-api-testconfig.h.in (+8/-0)
To merge this branch: bzr merge lp:~jpakkane/unity-scopes-api/randomstuff
Reviewer Review Type Date Requested Status
Michi Henning (community) Approve
Review via email: mp+166268@code.launchpad.net

Commit message

Various fixes.

Description of the change

Bunch of random stuff I found.

To post a comment you must log in.
Revision history for this message
Michi Henning (michihenning) wrote :

Thanks for the review!

One minor glitch:

link_directories(${UNITY_API_LIBRARY_DIRS})

I think that line should stay in. That's because it adds the location of libunit-api to the rpath when linking the binaries. Without it, the unity-api lib directory is not in the rpath which means, when I run the tests with a libunity-api that's somewhere in my staging area, and I've set PKG_CONFIG_PATH to point at the install location, the tests fail because they can't locate libunity-api.so at run time.

I'd prefer not to have to set LD_LIBRARY_PATH separately to make it work.

review: Needs Fixing
Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

I can see why that would be useful. However:

- CMake documentation recommends against using link_directories

- for consistency we should do this for every dependency

- Debian packagers discourage the use of rpath

- if you want to add stuff to rpath, it should be done explicitly with set_target_properties

- when you have the .so both in a system directory and a custom dir, LD_LIBRARY_PATH is the only sane way to select which one is chosen (and which can be changed without recompiling)

- the simple way of solving this is to have a daily build PPA of unity-api that you can use, which is the way most projects do it

If you still feel it should be there, I can add it back in.

Revision history for this message
Michi Henning (michihenning) wrote :

OK, you've convinced me. I'll live with setting LD_LIBRARY_PATH :-)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'CMakeLists.txt'
--- CMakeLists.txt 2013-05-29 08:51:07 +0000
+++ CMakeLists.txt 2013-05-29 14:36:31 +0000
@@ -63,8 +63,7 @@
63find_package(Boost 1.49.0 COMPONENTS regex REQUIRED)63find_package(Boost 1.49.0 COMPONENTS regex REQUIRED)
64pkg_check_modules(UNITY_API libunity-api REQUIRED)64pkg_check_modules(UNITY_API libunity-api REQUIRED)
65set(OTHER_INCLUDE_DIRS ${OTHER_INCLUDE_DIRS} ${UNITY_API_INCLUDE_DIRS})65set(OTHER_INCLUDE_DIRS ${OTHER_INCLUDE_DIRS} ${UNITY_API_INCLUDE_DIRS})
66set(OTHER_LIBS ${OTHER_LIBS} ${UNITY_API_LIBRARIES})66set(OTHER_LIBS ${OTHER_LIBS} ${UNITY_API_LDFLAGS})
67link_directories(${UNITY_API_LIBRARY_DIRS})
6867
69# Standard install paths68# Standard install paths
70include(GNUInstallDirs)69include(GNUInstallDirs)
@@ -113,7 +112,7 @@
113set(LIBS ${UNITY_SCOPES_LIB} ${OTHER_LIBS})112set(LIBS ${UNITY_SCOPES_LIB} ${OTHER_LIBS})
114113
115# Library install prefix114# Library install prefix
116set(LIB_INSTALL_PREFIX lib/${CMAKE_LIBRARY_ARCHITECTURE})115set(LIB_INSTALL_PREFIX "lib" CACHE PATH "Destination install dir for the library")
117116
118set(LIBDIR ${CMAKE_INSTALL_PREFIX}/${LIB_INSTALL_PREFIX} CACHE PATH "Destination install dir for the library")117set(LIBDIR ${CMAKE_INSTALL_PREFIX}/${LIB_INSTALL_PREFIX} CACHE PATH "Destination install dir for the library")
119set(libdir ${LIBDIR})118set(libdir ${LIBDIR})
120119
=== modified file 'test/CMakeLists.txt'
--- test/CMakeLists.txt 2013-05-29 04:57:50 +0000
+++ test/CMakeLists.txt 2013-05-29 14:36:31 +0000
@@ -1,3 +1,7 @@
1set(TEST_BUILD_ROOT ${CMAKE_CURRENT_BINARY_DIR})
2configure_file(scope-api-testconfig.h.in scope-api-testconfig.h @ONLY)
3include_directories(${CMAKE_CURRENT_BINARY_DIR})
4
1add_subdirectory(gtest)5add_subdirectory(gtest)
2add_subdirectory(headers)6add_subdirectory(headers)
3add_subdirectory(copyright)7add_subdirectory(copyright)
48
=== modified file 'test/gtest/unity/api/scopes/ScopeBase/CMakeLists.txt'
--- test/gtest/unity/api/scopes/ScopeBase/CMakeLists.txt 2013-05-29 04:57:50 +0000
+++ test/gtest/unity/api/scopes/ScopeBase/CMakeLists.txt 2013-05-29 14:36:31 +0000
@@ -1,6 +1,3 @@
1# This allows us to use a shared library in the current dir for testing without having to set LD_LIBRARY_PATH
2add_definitions(-DTESTLIBDIR=\"${CMAKE_CURRENT_BINARY_DIR}\")
3
4add_executable(ScopeBase_test ScopeBase_test.cpp)1add_executable(ScopeBase_test ScopeBase_test.cpp)
5target_link_libraries(ScopeBase_test ${LIBS} ${TESTLIBS})2target_link_libraries(ScopeBase_test ${LIBS} ${TESTLIBS})
63
74
=== modified file 'test/gtest/unity/api/scopes/ScopeBase/ScopeBase_test.cpp'
--- test/gtest/unity/api/scopes/ScopeBase/ScopeBase_test.cpp 2013-05-29 04:57:50 +0000
+++ test/gtest/unity/api/scopes/ScopeBase/ScopeBase_test.cpp 2013-05-29 14:36:31 +0000
@@ -19,7 +19,7 @@
19#include <unity/api/scopes/internal/ScopeBaseImpl.h>19#include <unity/api/scopes/internal/ScopeBaseImpl.h>
20#include <unity/api/scopes/internal/DynamicLoader.h>20#include <unity/api/scopes/internal/DynamicLoader.h>
21#include <unity/UnityExceptions.h>21#include <unity/UnityExceptions.h>
2222#include <scope-api-testconfig.h>
23#include <gtest/gtest.h>23#include <gtest/gtest.h>
2424
25using namespace unity::api::scopes;25using namespace unity::api::scopes;
@@ -27,7 +27,7 @@
2727
28namespace28namespace
29{29{
30 char const* scopelib = TESTLIBDIR "/libscopelib.so";30 char const* scopelib = TEST_BUILD_ROOT "/gtest/unity/api/scopes/ScopeBase/libscopelib.so";
31}31}
3232
33bool create_called = false;33bool create_called = false;
3434
=== modified file 'test/gtest/unity/api/scopes/internal/DynamicLoader/CMakeLists.txt'
--- test/gtest/unity/api/scopes/internal/DynamicLoader/CMakeLists.txt 2013-05-29 04:57:50 +0000
+++ test/gtest/unity/api/scopes/internal/DynamicLoader/CMakeLists.txt 2013-05-29 14:36:31 +0000
@@ -1,6 +1,3 @@
1# This allows us to use a shared library in the current dir for testing without having to set LD_LIBRARY_PATH
2add_definitions(-DTESTLIBDIR=\"${CMAKE_CURRENT_BINARY_DIR}\")
3
4add_executable(DynamicLoader_test DynamicLoader_test.cpp)1add_executable(DynamicLoader_test DynamicLoader_test.cpp)
5target_link_libraries(DynamicLoader_test ${LIBS} ${TESTLIBS})2target_link_libraries(DynamicLoader_test ${LIBS} ${TESTLIBS})
63
74
=== modified file 'test/gtest/unity/api/scopes/internal/DynamicLoader/DynamicLoader_test.cpp'
--- test/gtest/unity/api/scopes/internal/DynamicLoader/DynamicLoader_test.cpp 2013-05-29 04:57:50 +0000
+++ test/gtest/unity/api/scopes/internal/DynamicLoader/DynamicLoader_test.cpp 2013-05-29 14:36:31 +0000
@@ -20,15 +20,16 @@
20#include <unity/UnityExceptions.h>20#include <unity/UnityExceptions.h>
2121
22#include <gtest/gtest.h>22#include <gtest/gtest.h>
23#include <boost/regex.hpp> // BUGFIX: We don't use std::regex because, as of gcc 4.7.2, it is still broken23#include <boost/regex.hpp> // Use Boost implementation until http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53631 is fixed.
24#include <scope-api-testconfig.h>
2425
25using namespace std;26using namespace std;
26using namespace unity::api::scopes::internal;27using namespace unity::api::scopes::internal;
2728
28namespace29namespace
29{30{
30 char const* goodlib = TESTLIBDIR "/libtestlib.so";31 char const* goodlib = TEST_BUILD_ROOT "/gtest/unity/api/scopes/internal/DynamicLoader/libtestlib.so";
31 char const* badlib = TESTLIBDIR "/libbadtestlib.so";32 char const* badlib = TEST_BUILD_ROOT "/gtest/unity/api/scopes/internal/DynamicLoader/libbadtestlib.so";
32}33}
3334
34// Basic test.35// Basic test.
3536
=== added file 'test/scope-api-testconfig.h.in'
--- test/scope-api-testconfig.h.in 1970-01-01 00:00:00 +0000
+++ test/scope-api-testconfig.h.in 2013-05-29 14:36:31 +0000
@@ -0,0 +1,8 @@
1/* This file contains all configuration data needed to run unit tests. */
2
3#ifndef SCOPE_API_TESTCONFIG
4#define SCOPE_API_TESTCONFIG
5
6#define TEST_BUILD_ROOT "@TEST_BUILD_ROOT@"
7
8#endif
0\ No newline at end of file9\ No newline at end of file

Subscribers

People subscribed via source and target branches

to all changes: