i-rinat / libvdpau-va-gl

VDPAU driver with OpenGL/VAAPI backend
MIT License
159 stars 26 forks source link

Respect LIB_SUFFIX and LIB_INSTALL_DIR #4

Closed Nikoli closed 11 years ago

Nikoli commented 11 years ago

Ability to choose lib dir is required for making Gentoo package. Examples: http://quickgit.kde.org/?p=libechonest.git&a=blob&h=ac3c77b30febe9aa2f4127ecf5b51bf6974a6a81&hb=f32ec00185b3fbb4c493dc7cc511b0ad96b7911f&f=CMakeLists.txt#l69 https://bitbucket.org/acoustid/chromaprint/src/dd51f8e571227fc4bb8f09e382e4258ccea46757/CMakeLists.txt?at=master#cl-36

Fat-Zer commented 11 years ago

Here is an example patch for such options: https://gist.github.com/Fat-Zer/5845277 . btw: It would be nice if we could get moduledir from vdpau's pkg-config (like pkg-config vdpau --variable=moduledir), although FindPkgConfig haven't got such features.

i-rinat commented 11 years ago

fixed.

i-rinat commented 11 years ago

Here is an example patch for such options:

Thanks.

Fat-Zer commented 11 years ago

Sorry for my tedious exhortation, but you supposed to use CACHE variables for such stuff.

Firstly, this three lines:

if(NOT DEFINED LIB_INSTALL_DIR)
    set(LIB_INSTALL_DIR ${CMAKE_INSTALL_PREFIX}/lib${LIB_SUFFIX})
endif(NOT DEFINED LIB_INSTALL_DIR)

are equivalent to one:

set(LIB_INSTALL_DIR ${CMAKE_INSTALL_PREFIX}/lib${LIB_SUFFIX} CACHE PATH "<comment>" )

Moreover, the second variant is preferred cause the variable will be prompted in configuration utilities like cmake -I. Because of that it would be nice to give LIB_SUFFIX it's own declaration as well.

i-rinat commented 11 years ago

you supposed to use CACHE variables for such stuff.

Sounds reasonable. Applied.

Fat-Zer commented 11 years ago

LIB_INSTALL_DIR supposed to point to «lib» dir itself, not vdpau. AFAIK it's not the rule carved in stone but a lot of packages do so and some build systems [e.g. in gentoo] count on it.

i-rinat commented 11 years ago

LIB_INSTALL_DIR supposed to point to «lib» dir itself, not vdpau.

IMO, if build system sets custom LIB_INSTALL_DIR, that is the directory where it expects libraries to go. It's much more confusing to figure out that installer decided to append another directory (vdpau) to it. There is no sense to add another configurable variable either. If one have chosen to change LIB_INSTALL_DIR, he may add such suffix himself if needed.