Responses in line... Philip Lowman wrote:
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.
You're right, that should not be cached. I'll fix it in CVS.
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.
I guess I'll have to think about that for a bit. But really, if other modules are going to need to modify code to use the macro, shouldn't they move to the more modern and descriptive form while they're at it? I know they probably have to maintain backwards compatibility, but they can do that by using the macro and setting FOO_LIBRARY to FOO_LIBRARY_RELEASE after the macro is called.
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()
I'm not sure I understand the situation you're describing here. Are you referring to the older backwards compatibility case, or something about the usual FOO_LIBRARY_DEBUG and FOO_LIBRARY_RELEASE case?
Thanks for the feedback! -- Will -- Will Dicharry Software Developer Stellar Science Ltd Co
smime.p7s
Description: S/MIME Cryptographic Signature
_______________________________________________ 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
