> -----Original Message----- > From: cmake-developers [mailto:[email protected]] > On Behalf Of Kislinskiy, Stefan > Sent: Wednesday, August 26, 2015 10:03 > To: CHEVRIER, Marc; David Cole > Cc: [email protected] > Subject: Re: [cmake-developers] ExternalProject: Use native paths as > substitute for directory tokens > > I see. Added native-style path replacements for SOURCE_NATIVE_DIR and so > on. Also extended the documentation accordingly.
Some relatively major/minor/nitpicky thoughts in the interests of thoroughness: * There's no test code. It would be good if there was some code to explicitly test these options for nightly testing. For that matter, I think the original tokens weren't thoroughly tested either (e.g. a quick grep for "TMP_DIR" doesn't show usage of that token at all in the tests). The whole situation could therefore use some help, I think. There are several existing tests of ExternalProject that could probably be extended to test these tokens as needed. * Token replacement is done for WORKING_DIRECTORY and BYPRODUCTS parameters of add_custom_command. You'd best be sure those parameters of add_custom_command can handle a native path, or else not convert to native paths - even for <*_NATIVE> tokens - when the path is destined for one of those parameters. (I'd tend to assume the command *could* handle it properly since a lot of users might inadvertently pass them, but it's something that should be double-checked. If the command is passing those paths as-is to the generator, correct operation could even vary by generator.) The easy way out might be to have a parameter on _ep_replace_location_tags declaring whether to ACTUALLY convert native path tokens to native paths, and then callers to that macro can decide if they can / should handle native paths. * It would be useful if the documentation explicitly stated that the old <BINARY_DIR> & friends variables are using CMake paths; currently the documentation does not say - which was a shortcoming of the original docs (as you found out accidentally, it is CMake paths). * Nothing explicitly to do with your patch, but the documentation doesn't state that any of the tokens are valid in ExternalProject_Add; it may be useful to audit the options of ExternalProject_Add and see where they can be used, and document accordingly. (Some are obvious, like CONFIGURE_COMMAND, but some are less obvious, like CMAKE_CACHE_ARGS / CMAKE_CACHE_DEFAULT_ARGS which appear to have their own special handling for these tokens, yet none of that is documented. Even a newcomer might not realize the obvious, if they don't realize that ExternalProject_Add is implemented via ExternalProject_AddStep.) * New features should also be documented in Help/release/dev so that it makes it into the release notes of the next CMake version (i.e. so people notice your new feature). I leave it to others to decide if adding the new <*_NATIVE> tokens is the right way, or if there's a better way - as I myself am a newcomer to CMake. I think I like it though. :) Best regards, James Johnston -- 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
