On 02/25/2015 03:07 PM, Gregor Jasny wrote: > Changes since v5: > * Rebased against master (could drop two applied patches) > * kept help modules list sorted > * indirected xctest wiring
Great. The tests pass on my machine with no special configuration. The basic functionality is in good shape. My remaining comments are on the XCTestUtilities module that I hadn't fully reviewed before: * Please split addition of XCTestUtilities into its own commit between the current two with its own commit message. * Please use the ".. command:: ..." explicit markup to document the module API. That will make the functions indexed and linkable from other documentation. See the ExternalData module documentation for an example. * Please rename the APIs to start in "xctest_", e.g. add_xctest => xctest_add. We like to keep module-provided APIs prefixed with something related to the module name. > if(NOT CMAKE_OSX_SYSROOT) > message(STATUS "Adding XCTest bundles requires CMAKE_OSX_SYSROOT to be > set.") > endif() Should this be a FATAL_ERROR? > get_target_property(TESTEE_TYPE ${testee} TYPE) > get_target_property(TESTEE_FRAMEWORK ${testee} FRAMEWORK) > get_target_property(TESTEE_MACOSX_BUNDLE ${testee} MACOSX_BUNDLE) Please use get_property(... TARGET ${testee} ...) instead. We may one day deprecate the get_*_property commands because they are so inconsistent in how they report undefined properties. > find_library(FOUNDATION_LIBRARY Foundation) > if(NOT FOUNDATION_LIBRARY) > message(STATUS "Could not find Foundation Framework.") > endif() > > find_library(XCTEST_LIBRARY XCTest) > if(NOT XCTEST_LIBRARY) > message(STATUS "Could not find XCTest Framework.") > endif() Rather than polluting the cache with these, we could just trust that they exist. Is there any reason not to do: target_link_libraries(${target} PRIVATE "-framework Foundation" "-framework XCTest") instead? Let Xcode find its own frameworks. Alternatively these should be something that the XCTestUtilities module finds as part of include()ing it instead of calling find_library again on every invocation of the API. > execute_process( > COMMAND xcrun --find xctest > OUTPUT_VARIABLE XCTEST_EXECUTABLE > OUTPUT_STRIP_TRAILING_WHITESPACE) Use ERROR_VARIABLE here with a bogus local variable name just to hide any stderr output from the user running CMake. As above one could pre-find this outside of any API call. > if(NOT XCTEST_EXECUTABLE) > message(STATUS "Unable to finc xctest binary.") > endif() If you go with the pre-find approach then this can still be in the function because we only need the value there. However, shouldn't this be an error too? Thanks, -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers