On Tue, Feb 9, 2010 at 11:15 AM, Will Dicharry
<wdicha...@stellarscience.com> wrote:
> Mike Jackson wrote:
>>
>> Here is one I wrote for Expat:
>>
>>
>> ------8<------------------------------------------------
>> # - Find expat
>> # Find the native EXPAT headers and libraries.
>> #
>> # EXPAT_INCLUDE_DIRS - where to find expat.h, etc.
>> # EXPAT_LIBRARIES - List of libraries when using expat.
>> # EXPAT_LIBRARY_DEBUG - Debug version of Library
>> # EXPAT_LIBRARY_RELEASE - Release Version of Library
>> # EXPAT_FOUND - True if expat found.
>
> The macro below has been generalized and placed in the standard set of CMake
> modules with the 2.8 release. I saw it duplicated in enough places that I
> added it for general use in the Modules directory. It's called
> SelectLibraryConfigurations, you can get information about it by typing
> cmake --help-module SelectLibraryConfigurations. You don't have to use the
> standard one, but it's there if you need it. Let me know if you run into any
> trouble with it.
Thank you for adding this and mentioning it! There are probably lots
of modules out there that need to be modified to take advantage of
this.
One minor thing I don't like about the module is this line (70) right here:
set( ${basename}_LIBRARY ${${basename}_LIBRARY} CACHE FILEPATH "The
${basename} library")
Presumably users of this module have already called
find_library(${basename}_LIBRARY_RELEASE) and
find_library(${basename}_LIBRARY_DEBUG) since it's expecting these
names. Why then add a 3rd cache variable? If the user were to change
${basename}_LIBRARY_DEBUG after an initial configure, the cache
variable will not get fixed since set() does not operate on cache
variables that have already been initialized unless FORCE is used.
This isn't a major issue since you define a ${basename}_LIBRARY normal
variable which overrides the cache variable immediately above it. At
best, line 70 where you declare the CACHE variable is superfluous. It
could also be a little confusing to some users who change it in the
CACHE and see no effect.
===
My only other comment is that some modules already use the following
form (usually due to someone adding DEBUG support later)
FOO_LIBRARY (cache variable)
FOO_LIBRARY_DEBUG (cache variable)
FOO_LIBRARIES (normal variable with debug/optimized keywords when
FOO_LIBRARY_DEBUG is available, else normal variable set to
FOO_LIBRARY)
It would be nice to see a second macro that supports this format.
Also, the redefinition of FOO_LIBRARY to have optimized & release
keywords in it may or may not be desired. The only harm I can see
this causing is if a user were to have done this already in his
CMakeLists.txt. In this case they might find themselves with a CMake
error or linking against liboptimized.so! :)
if(FOO_LIBRARY_DEBUG)
target_link_libraries(bar optimized ${FOO_LIBRARY} debug
${FOO_LIBRARY_DEBUG})
else()
target_link_libraries(bar ${FOO_LIBRARY})
endif()
--
Philip Lowman
_______________________________________________
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://www.cmake.org/mailman/listinfo/cmake
