Merge lp:~compiz-team/compiz/compiz.fix_1082633 into lp:compiz/0.9.9

[Sam Spilsbury]

Set the include and link directories to non-relative paths, fix coverage target

(LP: #1082633)

Set the include and link directories to non-relative paths, fix coverage target

(LP: #1082633)

[lp:~compiz-team/compiz/compiz.fix_1082633]

3482. By Sam Spilsbury on 2012-11-25

Fix stupid typo

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3482
http://jenkins.qa.ubuntu.com/job/compiz-ci/264/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/264/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/264//rebuild/?

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

It works. But why the need to use absolute paths?

review: Needs Information

[Sam Spilsbury (smspillaz)]

Relative paths are more of a pain to maintain if we ever move things around.

COMPIZ_MAIN_SOURCE_DIR and COMPIZ_MAIN_BINARY_DIR are defined in the
parent scope, so they are guaranteed to be available as long as we are
in a subdirectory

On Mon, Nov 26, 2012 at 11:23 AM, Daniel van Vugt
<email address hidden> wrote:
> Review: Needs Information
>
> It works. But why the need to use absolute paths?
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1082633/+merge/136041
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

[Sam Spilsbury (smspillaz)]

Also, this:

link_directories (..)

is wrong, since it sets the link directory to the parent /source/
directory. Not that it was really a problem here, but if you ever
needed to link to something in the binary directory above it would
bite you at the wrong time.

Generally speaking it is good practise to always qualify paths with
CMAKE_CURRENT_SOURCE_DIR or a variable with an absolute path, ask
tvoss.

On Mon, Nov 26, 2012 at 12:46 PM, Sam Spilsbury <email address hidden> wrote:
> Relative paths are more of a pain to maintain if we ever move things around.
>
> COMPIZ_MAIN_SOURCE_DIR and COMPIZ_MAIN_BINARY_DIR are defined in the
> parent scope, so they are guaranteed to be available as long as we are
> in a subdirectory
>
> On Mon, Nov 26, 2012 at 11:23 AM, Daniel van Vugt
> <email address hidden> wrote:
>> Review: Needs Information
>>
>> It works. But why the need to use absolute paths?
>> --
>> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1082633/+merge/136041
>> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>
>
>
> --
> Sam Spilsbury

--
Sam Spilsbury

[Daniel van Vugt (vanvugt)]

On the contrary, the opposite is usually true. Relative .. paths are immune to changes above them. As for changes to directories at the same level, both approaches are equally fragile for assuming rect/region/timer etc always exist at the same level under COMPIZ_MAIN_SOURCE_DIR. However relative paths have the benefits of simplicity and never requiring some variable to be maintained.

Yes, the src/*/CMakeLists.txt need improving to solve this problem however using absolute paths via variables does not help the situation at all.

That said, this proposal only makes the situation slightly worse. Still needs major clean up later either way.

review: Approve

[Sam Spilsbury (smspillaz)]

On Mon, Nov 26, 2012 at 12:57 PM, Daniel van Vugt
<email address hidden> wrote:
> Review: Approve
>
> On the contrary, the opposite is usually true. Relative .. paths are immune to changes above them. As for changes to directories at the same level, both approaches are equally fragile for assuming rect/region/timer etc always exist at the same level under COMPIZ_MAIN_SOURCE_DIR. However relative paths have the benefits of simplicity and never requiring some variable to be maintained.

http://www.cmake.org/pipermail/cmake/2009-November/033051.html

> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1082633/+merge/136041
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

[Daniel van Vugt (vanvugt)]

I have not yet seen CMake mess up relative paths myself. If it does then that should be considered a bug.

All CMake has to do is translate a bunch of macros and dependencies into regular Make (GNU Make). And Make has always been designed to operate primarily on the current directory using relative paths. That's the rule rather than an exception. If CMake can't get this right then I'm a bit concerned for CMake.

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2012-11-14 10:17:09 +0000
+++ src/CMakeLists.txt	2012-11-25 04:44:19 +0000
@@ -1,6 +1,8 @@
+set (COMPIZ_MAIN_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+set (COMPIZ_MAIN_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
+
 include (CompizBcop)
 
-
 add_subdirectory( string )
 add_subdirectory( logmessage )
 add_subdirectory( timer )

=== modified file 'src/tests/CMakeLists.txt'
--- src/tests/CMakeLists.txt	2012-11-15 05:48:31 +0000
+++ src/tests/CMakeLists.txt	2012-11-25 04:44:19 +0000
@@ -3,17 +3,17 @@
 )
 
 include_directories (
-    ..
-    ../rect/include
-    ../region/include
-    ../timer/include
-    ../pluginclasshandler/include
-    ../window/geometry/include
-    ../window/extents/include
+    ${COMPIZ_MAIN_SOURCE_DIR}
+    ${COMPIZ_MAIN_SOURCE_DIR}/rect/include
+    ${COMPIZ_MAIN_SOURCE_DIR}/region/include
+    ${COMPIZ_MAIN_SOURCE_DIR}/timer/include
+    ${COMPIZ_MAIN_SOURCE_DIR}/pluginclasshandler/include
+    ${COMPIZ_MAIN_SOURCE_DIR}/window/geometry/include
+    ${COMPIZ_MAIN_SOURCE_DIR}/window/extents/include
     ${COMPIZ_INCLUDE_DIRS}
 )
 
-link_directories (..)
+link_directories (${COMPIZ_MAIN_BINARY_DIR})
 
 target_link_libraries (compiz_test_outputdevices
     -Wl,-start-group
@@ -27,5 +27,5 @@
     ${GTEST_BOTH_LIBRARIES}
 )
 
-compiz_discover_tests(compiz_test_outputdevices COVERAGE compiz_test_outputdevices)
+compiz_discover_tests(compiz_test_outputdevices COVERAGE compiz_core)