[cmake-developers] ExternalProject: Use native paths as substitute for directory tokens

[James Johnston]

> -----Original Message-----
> From: cmake-developers [mailto:cmake-developers-bounces at cmake.org]
> On Behalf Of Kislinskiy, Stefan
> Sent: Wednesday, August 26, 2015 10:03
> To: CHEVRIER, Marc; David Cole
> Cc: cmake-developers at cmake.org
> 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