Inconsistent behavior of cmake_path() subcommands taking a variable name instead of a string

Development
1

@marc.chevrier First up, sorry I didn’t get a chance to look at the original merge request for this feature when it was up for review. I should have caught this earlier, but am only just now getting to take a close look at this new command.

The new cmake_path() command being added for CMake 3.19 has subcommands of the form:

cmake_path(SUBCOMMAND <path> ...)

For all subcommands, the <path> is required to be the name of a variable and the command will read that variable. I’m concerned that for at least some of the subcommands, this is unintuitive. Consider the GET subcommand, which expects another option to specify the name of the output variable. For example:

cmake_path(GET <path> FILENAME <output>)

There doesn’t seem to be any reason why <path> should be expected to be a variable name in this case. To me, it would be more intuitive if <path> was a string, since this is how most other CMake commands work. If that were the case, I’d then write the docs for this command like so (here <path> now is not the name of a variable, but an actual string):

cmake_path(GET <path> FILENAME <outputVariable>)

Other subcommands modify a variable in-place. For those where this makes sense, they could be documented differently to highlight this. If a name of a variable is expected, then in the docs we should use naming that makes this clear. For example:

cmake_path(APPEND varName [<input>...])

Note that the original documentation for the above looks like this:

cmake_path(APPEND <path> [<input>...] [OUTPUT_VARIABLE <output>])

It looks like this command can’t decide whether it wants to modify the variable in-place or expects you to provide a variable to store the result in. Other CMake commands do one or the other, but here we are trying to support both. I think this will be a source of confusion for users and we should offer one or the other. Since we have list(APPEND) and friends as a precedent, one might consider following that example where it makes sense to modify a variable in place. Otherwise, require the name of the output variable to be explicitly provided and take a string rather than a variable name for the input.

I’ve just listed a couple of examples above, but I think we should revisit each of the subcommands and ensure we have behavior that is predictable and consistent with other CMake commands. I understand that these commands are likely based on the implementation of std::filesystem, but that’s an internal implementation detail and we should first aim to be consistent with existing CMake behaviours.

Cc: @ferdnyc

2

Hi Craig,

I globally disagree with your comments. On the contrary to your feeling I take care to have the more consistent behavior as possible. What I try to avoid is the total mess of the string() command:

  • some sub-commands required a string, others a variable.
  • placement of arguments varies. See for example the latest addition, i.e. JSON, expecting the output as first argument while the others expect the output as last argument.

My approach is fully similar the command list() which is, by far, the more consistent. So nearly all commands take a variable as input parameter (of course there is always some exceptions). And, similar to list(), sub-commands work in place except when it makes nonsense (again, same as list(): GET do not work in pace, but APPEND do).

Now, for convenience, I extend all sub-commands working in place to also support output result to avoid the following pattern, which I always found painful:

# I want to keep value of MY_DIR
set(MY_DIR "...")
set(MY_FILE1 "${MY_DIR")
cmake_path(APPEND MY_FILE1 "my_file1")
set (MY_FILE2 "${MY_DIR}")
cmake_path(APPEND MY_FILE2 "my_file2")

By using the possibility to work out of place, we get a more concise and comprehensive code:

set(MY_DIR "...")
cmake_path(APPEND MY_DIR "my_file1" OUTPUT_VARIABLE MY_FILE1)
cmake_path(APPEND MY_DIR "my_file2" OUTPUT_VARIABLE MY_FILE2)

And, again, I repeat, it is a commodity extension which do not remove any facility.

So, to conclude, I don’t think cmake_path() approach is not consistent with other commands. It is exactly the same approach as list() which is, I repeat, the more coherent and easy to use. I don’t have, contrary to the others, to refer to the documentation, again and again, because all sub-commands have consistent signatures.

Now for the documentation part, in the introduction, I explain what is the semantic of each place-holder, so I estimated it was not necessary to repeat this information for each sub-command. And for the names of place-holder, again, I use the same approach as list() which use place-holder <list>, not <listVariable>.
But I am not against any evolution of the documentation facilitating the understanding of the command.

3

I am in favor of changing <path> to <path_var> in the documentation on the presumption that people aren’t going to read the foreword either by skipping directly to Synopsis or having arrived via a hyperlink to a specific command.

4

Please move any further discussion to this issue in the issue tracker.