rdiankov / collada-dom

COLLADA Document Object Model (DOM) C++ Library
78 stars 40 forks source link

cmake: substitute _PREFIX explicitly during build #15

Closed morxa closed 9 years ago

morxa commented 9 years ago

Instead of setting _PREFIX to CMAKE_CURRENT_LIST_FILE, explicitly substitute _PREFIX with the value of CMAKE_INSTALL_PREFIX during the build process.

This avoids a bug where COLLADA_DOM_ROOT_DIR was set to '/' if the path contains '/lib', which results in COLLADA_DOM_INCLUDE_DIRS being '/include/collada-dom' instead of '/usr/include/collada-dom'.

morxa commented 9 years ago

Note: I'm not entirely sure what the line get_filename_component(_PREFIX "${_PREFIX}" PATH) is supposed to do. In my understanding, it will only set _PREFIX to its current value, as it is already a PATH. Also, if there is a case where _PREFIX should NOT be CMAKE_INSTALL_PREFIX, this won't work. Please check this before merging.

morxa commented 9 years ago

This was reported on the Red Hat Bugzilla #1057985

rdiankov commented 9 years ago

wow, seems like this has been pretty heavily discussed. the relative paths was just a good way of making the config script robust if the install dir changed, if we use CMAKE_INSTALL_PREFIX, we're shooting that out the door.

just curious, why not do the following?

set(COLLADA_DOM_ROOT_DIR "@CMAKE_INSTALL_PREFIX@")
morxa commented 9 years ago

I wasn't sure if anything else relied on an existing _PREFIX (although a fast grep didn't find anything), so I figured I'd leave it in.

Is it necessary to be able to move the installation without a rebuild? I guess that's exactly what caused the issue, and Orion's comment suggests that this is not really supported by cmake.

rdiankov commented 9 years ago

i guess it should be fine. can you make the commit so i can merge? thanks

morxa commented 9 years ago

done

rdiankov commented 9 years ago

thanks!

morxa commented 9 years ago

Thanks for merging!