Hello Brad,
thank you for the review. In addition to the last series I added a test
case also for the Makefile generator.
On 26/02/15 15:50, Brad King wrote:
> 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.
>
Everything addressed.
>> 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.
This approach works properly for Xcode. But when building with the
Makefile Generator the XCTest framework is not in the default compiler
search path (but in CMAKE_SYSTEM_FRAMEWORK_PATH).
So I could use a if(XCODE) conditional to avoid looking up the XCTest
framework. But consistency across generators is also a nice thing.
>> 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?
Also everything addressed.
Thanks,
Gregor
--
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
