[cmake-developers] CMakeParseArguments: Do not skip empty arguments

[Daniele E. Domenichelli]

Hello all,

Current implementation of cmake_parse_arguments skips empty arguments.

For example:

cmake_parse_arguments("OPTION" "SINGLE" "MULTI" ${ARGN})

with ARGN that is set using something like:

<args> SINGLE "" <more args> # (<args>;SINGLE;;<more args>)
<args> SINGLE "" # (<args>;SINGLE;)

it will fail in 3 different ways:

1) because the "foreach" skips empty arguments
2) because list(APPEND <list> ${currentArg}) will fail to append an
empty element to the list if currentArg is an empty string.
3) because empty arguments are not passed to cmake_parse_arguments
if not quoted, i.e. if ARGN="a;;b"
* ${ARGN} will pass the arguments "a", "b"
* "${ARGN}" will pass the arguments "a", "", "b"

I wrote a small patch to fix this, you can find it in the
CMakeParseArguments_EmptyArgs topic. This patch fixes the first
behaviour using foreach(IN LISTS) version, the second by enclosing
currentArg in quotes where required, and finally fixes the documentation
in order to suggest to use quoted "${ARGN}" in order not to miss the
empty arguments at the end of the list.

I don't know if this is to be considered a change of behaviour though,
but I'd rather consider it a bug, and I would like to see it fixed in
the next bugfix release... what do you think?


Cheers,
Daniele

[Alexander Neundorf]

Post by Daniele E. Domenichelli
Hello all,
Current implementation of cmake_parse_arguments skips empty arguments.
cmake_parse_arguments("OPTION" "SINGLE" "MULTI" ${ARGN})
<args> SINGLE "" <more args> # (<args>;SINGLE;;<more args>)
<args> SINGLE "" # (<args>;SINGLE;)
1) because the "foreach" skips empty arguments
2) because list(APPEND <list> ${currentArg}) will fail to append an
empty element to the list if currentArg is an empty string.
3) because empty arguments are not passed to cmake_parse_arguments
if not quoted, i.e. if ARGN="a;;b"
* ${ARGN} will pass the arguments "a", "b"
* "${ARGN}" will pass the arguments "a", "", "b"

Not sure about the third point. It could be done intentionally that way in
order to ignore empty strings.

Post by Daniele E. Domenichelli
I wrote a small patch to fix this, you can find it in the
CMakeParseArguments_EmptyArgs topic. This patch fixes the first
behaviour using foreach(IN LISTS) version, the second by enclosing
currentArg in quotes where required, and finally fixes the documentation
in order to suggest to use quoted "${ARGN}" in order not to miss the
empty arguments at the end of the list.
I don't know if this is to be considered a change of behaviour though,
but I'd rather consider it a bug, and I would like to see it fixed in
the next bugfix release... what do you think?

cmake_parse_arguments() is used in quite a few places in the meantime, so I
don't really feel good about changing its behaviour in general.
Maybe an option for the macro what it should do with empty values ?

Alex

[Daniele E. Domenichelli]

Post by Alexander Neundorf

Post by Daniele E. Domenichelli
I don't know if this is to be considered a change of behaviour though,
but I'd rather consider it a bug, and I would like to see it fixed in
the next bugfix release... what do you think?

cmake_parse_arguments() is used in quite a few places in the meantime, so I
don't really feel good about changing its behaviour in general.
Maybe an option for the macro what it should do with empty values ?

Is there any of the other place where cmake_parse_arguments() has to
deal with with empty arguments? I believe that someone would have
complained about that.

[Alexander Neundorf]

Post by Daniele E. Domenichelli

Post by Alexander Neundorf

Post by Daniele E. Domenichelli
I don't know if this is to be considered a change of behaviour though,
but I'd rather consider it a bug, and I would like to see it fixed in
the next bugfix release... what do you think?

cmake_parse_arguments() is used in quite a few places in the meantime, so
I don't really feel good about changing its behaviour in general.
Maybe an option for the macro what it should do with empty values ?

Is there any of the other place where cmake_parse_arguments() has to
deal with with empty arguments?

We cannot know what users are doing with it.

Post by Daniele E. Domenichelli
I believe that someone would have
complained about that.

[Brad King]

Adding proper named argument handling to cmake_parse_arguments() itself is
somewhat complicated since it can't make use of cmake_parse_arguments() ;-)

Since the need for this is so common, perhaps we should provide a
C++-implemented command to do it, e.g. cmake_command_options().
We would need to carefully design the interface here first of course.

-Brad

[Matthew Woehlke]

Post by Brad King

Adding proper named argument handling to cmake_parse_arguments() itself is
somewhat complicated since it can't make use of cmake_parse_arguments() ;-)

Since the need for this is so common, perhaps we should provide a
C++-implemented command to do it, e.g. cmake_command_options().
We would need to carefully design the interface here first of course.

FWIW, I prefer the name [cmake_]parse_arguments :-).

Is there not already a C++ implementation something like this? (Or else
how do the existing C++ commands do argument parsing?) If it is C++, I
guess it would be possible for it to not have the limitation of being
unable to parse its own arguments? (Actually, in either case, it seems
that the implementation should be able to have all the 'real work' in a
helper that is called once to parse arguments to the command itself,
then again to parse the arguments the user wants to parse.)

In any case, it would be nice if we could reimplement
cmake_parse_arguments so that it has the same signature but the
implementation would newly just wrap the new C++ version.

What if we had something like:

parse_arguments(
PREFIX "_"
FLAG_OPTIONS ...
VALUE_OPTIONS ...
LIST_OPTIONS ...
ARGS ...)

...where everything after ARGS is no longer considered an argument to
parse_arguments itself. Also, 'parse_arguments(... IN LISTS ...)'
(instead of using 'ARGS'), taking names of list variables containing
arguments to be parsed (similar to 'IN LISTS' of foreach).

{FLAG,VALUE,LIST}_OPTIONS would accept one or more strings containing
names of argument keywords, subject to recursive list expansion (should
be safe - keywords should not contain ';' - and avoids breakage if
option name lists are given in quoted variable expansions, which could
easily happen by forgetting to remove quotes when porting to the new
signature).

...and then of course cmake_parse_arguments can readily accept other
options to modify its behavior e.g. KEEP_EMPTY (and/or SKIP_EMPTY if
keeping becomes - or may be, depending on policy - the default).

[Petr Kmoch]

Hi all.

I hope I don't derail the conversation, but since there's now discussion of
providing a new interface and perhaps behaviour-changing parameters for
cmake_parse_arguments(), I'd like to mention another behaviour switch I
would find useful.

I maintain a CMake framework for a large collection of projects at work,
and this framework provides several functions which (naturally) use
cmake_parse_arguments() internally. My users (and myself when I first saw
it) were quite surprised that repeating the leading keyword for multi-value
parameters overwrites values instead of appending them. What I mean:

function(foo)
cmake_parse_arguments(arg "" "" "MULTI" ${ARGN})
foreach(item IN LISTS arg_MULTI)
message(STATUS "${item}")
endforeach()
endfunction()

foo(MULTI x y MULTI z w)

The above code outputs 'z' and 'w'. I originally expected it to output all
of 'x' 'y' 'z' 'w'. I would certainly welcome an option to switch the
behaviour to the latter one.

Petr

Post by Matthew Woehlke

Post by Brad King

Adding proper named argument handling to cmake_parse_arguments() itself is
somewhat complicated since it can't make use of cmake_parse_arguments() ;-)

Since the need for this is so common, perhaps we should provide a
C++-implemented command to do it, e.g. cmake_command_options().
We would need to carefully design the interface here first of course.

FWIW, I prefer the name [cmake_]parse_arguments :-).
Is there not already a C++ implementation something like this? (Or else
how do the existing C++ commands do argument parsing?) If it is C++, I
guess it would be possible for it to not have the limitation of being
unable to parse its own arguments? (Actually, in either case, it seems that
the implementation should be able to have all the 'real work' in a helper
that is called once to parse arguments to the command itself, then again to
parse the arguments the user wants to parse.)
In any case, it would be nice if we could reimplement
cmake_parse_arguments so that it has the same signature but the
implementation would newly just wrap the new C++ version.
parse_arguments(
PREFIX "_"
FLAG_OPTIONS ...
VALUE_OPTIONS ...
LIST_OPTIONS ...
ARGS ...)
...where everything after ARGS is no longer considered an argument to
parse_arguments itself. Also, 'parse_arguments(... IN LISTS ...)' (instead
of using 'ARGS'), taking names of list variables containing arguments to be
parsed (similar to 'IN LISTS' of foreach).
{FLAG,VALUE,LIST}_OPTIONS would accept one or more strings containing
names of argument keywords, subject to recursive list expansion (should be
safe - keywords should not contain ';' - and avoids breakage if option name
lists are given in quoted variable expansions, which could easily happen by
forgetting to remove quotes when porting to the new signature).
...and then of course cmake_parse_arguments can readily accept other
options to modify its behavior e.g. KEEP_EMPTY (and/or SKIP_EMPTY if
keeping becomes - or may be, depending on policy - the default).
--
Matthew
--
Powered by www.kitware.com
Visit other Kitware open-source projects at http://www.kitware.com/
opensource/opensource.html
http://www.cmake.org/Wiki/CMake_FAQ
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers

[Daniele E. Domenichelli]

Hello,

I updated the CMakeParseArguments_EmptyArgs topic, the new behaviour is
now decided as follows:

* If CMAKE_MINIMUM_REQUIRED_VERSION < 2.8.13, the default behaviour is
to skip empty arguments, otherwise the default behaviour is to keep
them.
* Using the CMAKE_PARSE_ARGUMENTS_DEFAULT_SKIP_EMPTY directory
property the user can explicitly set the default behaviour for a
folder and its subfolders.
* Using CMAKE_PARSE_ARGUMENTS_(SKIP|KEEP)_EMPTY or as the first
argument of the call after the mandatory ones, the user can specify
whether to keep or to skip the empty arguments, regardless of the
default behaviour for that cmake_parse_arguments() call.

Therefore the new syntax is:

cmake_parse_arguments(
<prefix>
<options>
<one_value_keywords>
<multi_value_keywords>
[CMAKE_PARSE_ARGUMENTS_(SKIP|KEEP)_EMPTY]
args...
)

Does it look better now?


Unfortunately there still a big problem, that is probably in the c++
source code, because "set(<var> "<value>" PARENT_SCOPE)" still fails if
<value> is empty. This is a small example that shows the problem:

---

function(_foo)
set(FOO "" PARENT_SCOPE)
endfunction()

foo(FOO "foo")
_foo()
if(DEFINED FOO)
message("FOO DEFINED")
else()
message("FOO NOT DEFINED")
endif()

set(BAR "")
if(DEFINED BAR)
message("BAR DEFINED")
else()
message("BAR NOT DEFINED")
endif()

---

The output of this is:

FOO NOT DEFINED
BAR DEFINED

So it looks like that "set(FOO "" PARENT_SCOPE)" instead of setting an
empty FOO value, unsets it in the parent scope, and the behaviour is
definitely different from set for the current scope.

This looks like a bug to me... How should I handle this?


Cheers,
Daniele

[Daniele E. Domenichelli]

Post by Daniele E. Domenichelli
foo(FOO "foo")

Obviously it was supposed to be

set(FOO "foo")

Sorry...


Daniele

[Alexander Neundorf]

Post by Daniele E. Domenichelli
Hello,
I updated the CMakeParseArguments_EmptyArgs topic, the new behaviour is
* If CMAKE_MINIMUM_REQUIRED_VERSION < 2.8.13, the default behaviour is
to skip empty arguments, otherwise the default behaviour is to keep
them.
* Using the CMAKE_PARSE_ARGUMENTS_DEFAULT_SKIP_EMPTY directory
property the user can explicitly set the default behaviour for a
folder and its subfolders.
* Using CMAKE_PARSE_ARGUMENTS_(SKIP|KEEP)_EMPTY or as the first
argument of the call after the mandatory ones, the user can specify
whether to keep or to skip the empty arguments, regardless of the
default behaviour for that cmake_parse_arguments() call.
cmake_parse_arguments(
<prefix>
<options>
<one_value_keywords>
<multi_value_keywords>
[CMAKE_PARSE_ARGUMENTS_(SKIP|KEEP)_EMPTY]
args...
)
Does it look better now?
Unfortunately there still a big problem, that is probably in the c++
source code, because "set(<var> "<value>" PARENT_SCOPE)" still fails if
---
function(_foo)
set(FOO "" PARENT_SCOPE)
endfunction()
foo(FOO "foo")
_foo()
if(DEFINED FOO)
message("FOO DEFINED")
else()
message("FOO NOT DEFINED")
endif()
set(BAR "")
if(DEFINED BAR)
message("BAR DEFINED")
else()
message("BAR NOT DEFINED")
endif()
---
FOO NOT DEFINED
BAR DEFINED
So it looks like that "set(FOO "" PARENT_SCOPE)" instead of setting an
empty FOO value, unsets it in the parent scope, and the behaviour is
definitely different from set for the current scope.
This looks like a bug to me... How should I handle this?

the implementation in set() doesn't differentiate whether it was set to empty
or whether there was no value at all.
In most cases it doesn't matter much whether a variable is empty or not
defined, if it is dereferenced it gives an empty string in both cases, if()
also interprets both as false.
It's more or less only if(DEFINED) which detects the difference, right ?

How about adding one new policy "modify handling of empty variables" which is
used by both ?
Then you don't need the directory property and the extra argument.
Having two separate policies would be cleaner, but it seems a bit overkill to
me, since most probably both changes won't break many, if any, builds.

Alex

[Brad King]

Post by Alexander Neundorf

Post by Daniele E. Domenichelli
So it looks like that "set(FOO "" PARENT_SCOPE)" instead of setting an
empty FOO value, unsets it in the parent scope, and the behaviour is
definitely different from set for the current scope.

How about adding one new policy "modify handling of empty variables" which is
used by both ?

At most the policy should affect PARENT_SCOPE. One can see the
difference right in cmSetCommand, where parentScope was added:

http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=fc8ce174#patch6

For parentScope it does

if (value.empty())
{
this->Makefile->RaiseScope(variable, 0);
}
else
{
this->Makefile->RaiseScope(variable, value.c_str());
}

while for normal scope it does just

this->Makefile->AddDefinition(variable, value.c_str());

-Brad

[Alexander Neundorf]

Post by Brad King

Post by Alexander Neundorf

Post by Daniele E. Domenichelli
So it looks like that "set(FOO "" PARENT_SCOPE)" instead of setting an
empty FOO value, unsets it in the parent scope, and the behaviour is
definitely different from set for the current scope.

How about adding one new policy "modify handling of empty variables"
which is used by both ?

At most the policy should affect PARENT_SCOPE.

Yes.
I meant "both" set(PARENT_SCOPE) and cmake_parse_arguments().

Alex

[Daniele E. Domenichelli]

Hello,

I updated the CMakeParseArguments_EmptyArgs topic, so that it will work
properly with cmake 3.0.0 (after the branch set_emptyvar_PARENT_SCOPE
topic is merged) and with older releases (I had to add a workaround for
that)

Can you please review it?

Thanks!

Daniele

[Daniele E. Domenichelli]

Post by Daniele E. Domenichelli
I updated the CMakeParseArguments_EmptyArgs topic, so that it will work
properly with cmake 3.0.0 (after the branch set_emptyvar_PARENT_SCOPE
topic is merged) and with older releases (I had to add a workaround for
that)
Can you please review it?

Any comment?
Can I merge it to next?


Daniele

[Brad King]

Post by Daniele E. Domenichelli
Any comment?

I was hoping others that use this module would respond here.

Post by Daniele E. Domenichelli
Can I merge it to next?

Go ahead and merge it to 'next' for testing. I will review it
there when I return from vacation next week.

Thanks,
-Brad

[Jean-Christophe Fillion-Robin]

Hi Danielle,

We use the macro a lot in project like Slicer and CTK.
This is a great improvement of the "CMakeParseArguments" module. Thanks for
working on this.

Jc

Post by Brad King

Post by Daniele E. Domenichelli
Any comment?

I was hoping others that use this module would respond here.

Post by Daniele E. Domenichelli
Can I merge it to next?

Go ahead and merge it to 'next' for testing. I will review it
there when I return from vacation next week.
Thanks,
-Brad
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
http://www.cmake.org/Wiki/CMake_FAQ
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers

[Brad King]

Post by Brad King
Go ahead and merge it to 'next' for testing. I will review it
there

Looking at the topic as of commit 10fd6fba, here are some comments.

Why does CMAKE_PARSE_ARGUMENTS_DEFAULT_SKIP_EMPTY need to be an
inherited directory property instead of a variable? Both have
essentially the same scope and variables are much more common.
I'd like to avoid new uses of define_property if possible. It
is a legacy from the old builtin documentation system.

I see this in the tests:

+# run_cmake(VERSION-KEEP) # Enable when 3.0.0 is released

We need to be able to test this even prior to the 3.0 release.
No one is going to remember to enable that as part of the release
process, and what if it is broken by then? You might be able to
hack the test with

set(CMAKE_MINIMUM_REQUIRED_VERSION 3.0.0)

after the real cmake_minimum_required call.

Stepping back for a moment: This macro is now a huge amount of
CMake code to do keyword argument processing for each invocation
of the calling macro or function. Accumulation of such uses adds
overhead to CMake language processing. It was previously proposed
in this thread to convert the implementation of the command to be
in C++. IMO this is still worth investigating, if only as a follow
up to this topic.

Thanks,
-Brad

[Daniele E. Domenichelli]

Post by Brad King
Why does CMAKE_PARSE_ARGUMENTS_DEFAULT_SKIP_EMPTY need to be an
inherited directory property instead of a variable? Both have
essentially the same scope and variables are much more common.
I'd like to avoid new uses of define_property if possible. It
is a legacy from the old builtin documentation system.

Ok, fixed... I saw it used in ExternalProject and I thought it was
something "standard".

Post by Brad King
We need to be able to test this even prior to the 3.0 release.
No one is going to remember to enable that as part of the release
process, and what if it is broken by then? You might be able to
hack the test with
set(CMAKE_MINIMUM_REQUIRED_VERSION 3.0.0)
after the real cmake_minimum_required call.

Ok thanks, fixed.


I fixed these issues and merged in next again.

Post by Brad King
Stepping back for a moment: This macro is now a huge amount of
CMake code to do keyword argument processing for each invocation
of the calling macro or function. Accumulation of such uses adds
overhead to CMake language processing. It was previously proposed
in this thread to convert the implementation of the command to be
in C++. IMO this is still worth investigating, if only as a follow
up to this topic.

+1, since this is a very useful feature.

But still it the module should be there for compatibility for
compatibility with scripts already using that, and the old
implementation should be there too in order to be able to backport the
module with older cmake versions...

Perhaps the implementation of the module could be changed to something like

if(NOT CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.0.0)
<call c++ command>
else()
<current implementation>
endif()

Also all the modules that you should be able to backport should have
something like

if(NOT CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.0.0)
<call c++ command>
else()
<current argument parsing implementation>
endif()


Cheers,
Daniele

[Brad King]

Post by Daniele E. Domenichelli

Post by Brad King
Stepping back for a moment: This macro is now a huge amount of
CMake code to do keyword argument processing for each invocation
of the calling macro or function. Accumulation of such uses adds
overhead to CMake language processing. It was previously proposed
in this thread to convert the implementation of the command to be
in C++. IMO this is still worth investigating, if only as a follow
up to this topic.

+1, since this is a very useful feature.

Actually after thinking about this over night I realized that converting
to a C++ implementation is the best way to fix the empty argument handling
too. The use of "CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.0.0" and
the directory-scoped CMAKE_PARSE_ARGUMENTS_DEFAULT_SKIP_EMPTY setting for
compatibility are an approximation of a CMake Policy. We should just use
a policy for it instead so that proper warnings can be given (when empty
arguments are passed) about the change in behavior. While it is possible
to implement a policy in CMake-language code it is much easier in C++ code.
Converting to C++ will have other benefits already discussed.

Post by Daniele E. Domenichelli
But still it the module should be there for compatibility for
compatibility with scripts already using that, and the old
implementation should be there too in order to be able to backport the
module with older cmake versions...

The module can be provided as an empty file except for documentation.
Any project including it will expect to be able to call cmake_parse_arguments
afterwards, which they will be able to since it will be a builtin command.
With the policy handling compatibility there is no need to keep the old
macro implementation. If a project has its own copy of the module it
will simply replace the builtin command and get the original behavior.

-Brad

[Daniele E. Domenichelli]

Post by Brad King
The module can be provided as an empty file except for documentation.
Any project including it will expect to be able to call cmake_parse_arguments
afterwards, which they will be able to since it will be a builtin command.
With the policy handling compatibility there is no need to keep the old
macro implementation. If a project has its own copy of the module it
will simply replace the builtin command and get the original behavior.

I was assuming for some reason that the c++ command would have been
"parse_arguments" instead of "cmake_parse_arguments", probably because
the only "cmake_" commands are strictly related to cmake.

Anyway, even with the same name if someone needs the empty arguments
parsing with an older cmake version, he will have to write it from
scratch if the version in cmake is empty...

Also all the modules will either not include CMakeParseArguments and use
the system command, or (according to what I believe it's current policy)
will include it from ${CMAKE_CURRENT_LIST_DIR}, and therefore the empty
one will be included

so perhaps it could be exactly as it is now in my topic:

if(COMMAND cmake_parse_arguments)
return()
endif()
<old implementation>


Daniele

[Brad King]

Post by Daniele E. Domenichelli
Anyway, even with the same name if someone needs the empty arguments
parsing with an older cmake version, he will have to write it from
scratch if the version in cmake is empty...

Do you mean he won't be able to copy it out of a newer CMake into his
project to use with an older CMake? IMO that should not influence our
upstream decisions. If you need this you can take the impl you have
in the topic now and use it elsewhere.

Post by Daniele E. Domenichelli
Also all the modules will either not include CMakeParseArguments and use
the system command, or (according to what I believe it's current policy)
will include it from ${CMAKE_CURRENT_LIST_DIR}, and therefore the empty
one will be included

CMake's own modules can be updated to not include the legacy module.
Project code will include it and it will do nothing. I do not see
a problem.

-Brad

[Brad King]

Post by Brad King

Post by Daniele E. Domenichelli
+1, since this is a very useful feature.

Actually after thinking about this over night I realized that converting
to a C++ implementation is the best way to fix the empty argument handling
too. The use of "CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.0.0" and
the directory-scoped CMAKE_PARSE_ARGUMENTS_DEFAULT_SKIP_EMPTY setting for
compatibility are an approximation of a CMake Policy. We should just use
a policy for it instead so that proper warnings can be given (when empty
arguments are passed) about the change in behavior. While it is possible
to implement a policy in CMake-language code it is much easier in C++ code.
Converting to C++ will have other benefits already discussed.

Without a policy project authors will not be warned about the change in
behavior that would be caused by bumping cmake_minimum_required(VERSION).
With a policy we would be able to fix the NEW behavior test cases to just
set the policy instead of hacking the CMAKE_MINIMUM_REQUIRED_VERSION value.

Currently I do not plan to merge the CMakeParseArguments_EmptyArgs topic or
any topics that depend on it (like ExternalProject-independent-step-targets)
to master. I'm convinced that the cmake_parse_arguments command should be
converted to C++, taught to handle empty arguments, and handle compatibility
with a policy. Daniele, will you be able to work on this?

Thanks,
-Brad

[Daniele E. Domenichelli]

Post by Brad King
Without a policy project authors will not be warned about the change in
behavior that would be caused by bumping cmake_minimum_required(VERSION).

Ok, let's assume that we add a C++ implementation with a CMPXXXX policy
that does
* OLD policy = SKIP_EMPTY
* NEW policy = KEEP_EMPTY

If CMAKE_MINIMUM_REQUIRED_VERSION < 3.0.0 and CMAKE_VERSION >= 3.0.0,
the author will receive a warning about the policy and that the OLD
policy is being used.

If CMAKE_MINIMUM_REQUIRED_VERSION >= 3.0.0, the NEW policy will be used,
but the author should also be warned that he should no longer include
the CMakeParseArguments.cmake file, since it will be deprecated and
might disappear in the future. I'm not sure if from C++ you can check
that, so an "empty cmake file" won't work...

If CMAKE_MINIMUM_REQUIRED_VERSION < 3.0.0 and CMAKE_VERSION < 3.0.0, it
means that the module was "backported", the OLD policy should be used
and no warning issued.


So, what about something like this:

---
### Documentation ###

### Copyright statement ###

if(NOT CMAKE_MINIMUM_REQUIRED_VERSION VERSION_LESS 3.0.0)
message(AUTHOR_WARNING "cmake_parse_arguments is now a builtin
function, the CMakeParseArguments is deprecated and could be
removed in the future, therefore it should not be included")
# Use C++ implementation
return()
endif()

if(COMMAND cmake_parse_arguments)
# If CMAKE_VERSION >= 3 will use the C++ implementation, and
# therefore will use the policy and when used will print a warning
# about the policy.
# If CMAKE_VERSION < 3 this file was already included and will avoid
# including several times this file.
return()
endif()

# The implementation is left for backwards compatibility, but might
# be removed in the future.
### CMake implementation for CMake < 3.0.0 simplified to use the ###
### OLD policy only (SKIP_EMPTY), but that accepts the ###
### CMAKE_PARSE_ARGUMENTS_KEEP_EMPTY argument and ignores the ###
### CMAKE_PARSE_ARGUMENTS_SKIP_EMPTY one ###
---

Does this sound reasonable? Sorry if I stress on keeping the CMake
implementation, but I need a solution that could be backported at least
to CMake 2.8.7 (that's the best I was able to achieve, we depended on
CMake 2.4 a few months ago in order to support very old systems, and for
sure I won't be able to change the dependency on CMake 3.0.0 any time
soon), and I hate the idea of "forking" and maintaining my own patched
version of the CMakeParseArguments module...


Are you sure you don't want the command to be renamed to
"parse_arguments"? The only commands containing "cmake" looks strictly
related to "cmake", and the arguments parsing does not look that much
related...

Post by Brad King
Currently I do not plan to merge the CMakeParseArguments_EmptyArgs topic or
any topics that depend on it (like ExternalProject-independent-step-targets)
to master.

Please note that ExternalProject does not use CMakeParseArguments and
the ExternalProject-independent-step-targets topic does not depend on
this patch, so I don't see a reason for blocking it on this topic.

Actually ExternalProject, like several other modules, would probably
benefit on using the C++ implementation instead of having their own
parsing, or using the cmake implementation, but that's for a following
patch.

Post by Brad King
I'm convinced that the cmake_parse_arguments command should be
converted to C++, taught to handle empty arguments, and handle compatibility
with a policy. Daniele, will you be able to work on this?

I will try, but I'll have to do it in my free time, and I don't know how
long it will take, because I'm not very confident with CMake "C++" code,
because my time very limited at the moment, and because I have several
other patches that I want to work on. If anyone else is in a hurry to
have this, and wants to take it, feel free...


Cheers,
Daniele

[Matthew Woehlke]

Post by Daniele E. Domenichelli
Are you sure you don't want the command to be renamed to
"parse_arguments"? The only commands containing "cmake" looks strictly
related to "cmake", and the arguments parsing does not look that much
related...

FWIW, I was sort-of hoping it would be. If so, CMakeParseArguments.cmake
can be left with a simple stub to call the new version.

As a bonus, the new version could itself take named arguments instead of
positional with a flag whether or not to skip empty (default = keep)
with the compatibility wrapper instead specifying to skip, and no policy
would be needed (if you want the new behavior, just use parse_arguments).

[Brad King]

Post by Matthew Woehlke

Post by Daniele E. Domenichelli
Are you sure you don't want the command to be renamed to
"parse_arguments"? The only commands containing "cmake" looks strictly
related to "cmake", and the arguments parsing does not look that much
related...

FWIW, I was sort-of hoping it would be. If so, CMakeParseArguments.cmake
can be left with a simple stub to call the new version.
As a bonus, the new version could itself take named arguments instead of
positional with a flag whether or not to skip empty (default = keep)
with the compatibility wrapper instead specifying to skip, and no policy
would be needed (if you want the new behavior, just use parse_arguments).

The C++-implemented command would be able to handle both the existing
positional or a new proposed keyword-based signature. The name
"parse_arguments" is not specific enough about what kind of arguments
it parses. By keeping the name as "cmake_parse_arguments" it indicates
that it parses cmake-language arguments, though another name such as
"process_cmake_command_arguments" would do that too.

An advantage of keeping the name is that existing callers get the
speed-up immediately. Furthermore there will be less code left in the
old module to maintain.

Matthew, would you have time to work on the C++ impl? I think we could
start with reproducing the current signature. Optionally add the policy
for handling empty arguments. Then add a proposed keyword-based sig
that handles empty arguments always.

Thanks,
-Brad

[Matthew Woehlke]

Post by Brad King

Post by Matthew Woehlke

Post by Daniele E. Domenichelli
Are you sure you don't want the command to be renamed to
"parse_arguments"? The only commands containing "cmake" looks strictly
related to "cmake", and the arguments parsing does not look that much
related...

FWIW, I was sort-of hoping it would be. If so, CMakeParseArguments.cmake
can be left with a simple stub to call the new version.
As a bonus, the new version could itself take named arguments instead of
positional with a flag whether or not to skip empty (default = keep)
with the compatibility wrapper instead specifying to skip, and no policy
would be needed (if you want the new behavior, just use parse_arguments).

The C++-implemented command would be able to handle both the existing
positional or a new proposed keyword-based signature. The name
"parse_arguments" is not specific enough about what kind of arguments
it parses. By keeping the name as "cmake_parse_arguments" it indicates
that it parses cmake-language arguments, though another name such as
"process_cmake_command_arguments" would do that too.

Sure, I can see the argument. I'm not sure I'm entirely sold on
'cmake_parse_arguments', but 'parse_cmake[_command]_arguments' makes
perfect sense to me. (IMHO 'parse' makes more sense than 'process'...
the command doesn't actually do anything with the arguments as might be
implied with 'process'; just extracts them, which to me is 'parsing'.)

I could go either way on 'cmake' vs. 'cmake_command'... the brevity of
the former is nice, while the specificity/clarity of the latter is also
nice. (Well, maybe not; I think I am actually leaning toward the latter
:-).)

Post by Brad King
An advantage of keeping the name is that existing callers get the
speed-up immediately. Furthermore there will be less code left in the
old module to maintain.

I think that is still true if the module just wraps the new function?

Post by Brad King
Matthew, would you have time to work on the C++ impl? I think we could
start with reproducing the current signature. Optionally add the policy
for handling empty arguments. Then add a proposed keyword-based sig
that handles empty arguments always.

If someone can sponsor it, maybe? Not for a few days at least, though.

[Brad King]

Post by Matthew Woehlke

Post by Brad King
An advantage of keeping the name is that existing callers get the
speed-up immediately. Furthermore there will be less code left in the
old module to maintain.

I think that is still true if the module just wraps the new function?

Yes, I think so. Be sure that the wrapper forwards empty arguments
correctly or the compatibility policy will not be meaningful ;)

Thanks,
-Brad

[Brad King]

Post by Daniele E. Domenichelli
If CMAKE_MINIMUM_REQUIRED_VERSION >= 3.0.0, the NEW policy will be used,
but the author should also be warned that he should no longer include
the CMakeParseArguments.cmake file, since it will be deprecated and
might disappear in the future. I'm not sure if from C++ you can check
that, so an "empty cmake file" won't work...

Well, almost empty :)

Post by Daniele E. Domenichelli
Does this sound reasonable? Sorry if I stress on keeping the CMake
implementation, but I need a solution that could be backported at least
to CMake 2.8.7

I see no reason to distribute code in the CMake 3.0 version of the
module that is meant to work with versions prior to 3.0. It will
never be tested or executed while running that version of CMake.
It would be an untested fork even though it appears upstream.

If you're going to use it with CMake 2.8.7 you will need a copy with
your project anyway. There is no reason you can't take your current
version of the module from this topic and put it in your project with
no change to CMake upstream.

Post by Daniele E. Domenichelli
Are you sure you don't want the command to be renamed to
"parse_arguments"? The only commands containing "cmake" looks strictly
related to "cmake", and the arguments parsing does not look that much
related...

I'll respond to this part on top of Matthew's response to it.

Post by Daniele E. Domenichelli
Please note that ExternalProject does not use CMakeParseArguments and
the ExternalProject-independent-step-targets topic does not depend on
this patch, so I don't see a reason for blocking it on this topic.

Ah, I was confused because ExternalProject-independent-step-targets
merges CMakeParseArguments_EmptyArgs. I see it does so only for a
conflict resolution. I can resolve that as described below.

Post by Daniele E. Domenichelli

Post by Brad King
I'm convinced that the cmake_parse_arguments command should be
converted to C++, taught to handle empty arguments, and handle compatibility
with a policy. Daniele, will you be able to work on this?

I will try, but I'll have to do it in my free time, and I don't know how
long it will take, because I'm not very confident with CMake "C++" code,

If you are not comfortable trying this there is no requirement to
do so. Others have expressed interested in this thread and may be
able to volunteer.

Based on the preceding discussion I do not think we will be changing the
upstream module until the C++ implementation is done. Therefore I
will revert the CMakeParseArguments_EmptyArgs topic (which will also
help in separating ExternalProject-independent-step-targets from it).

See my follow-up post in response to Matthew for further discussion
of a C++ implementation.

Thanks,
-Brad

[Brad King]

Post by Daniele E. Domenichelli
Actually ExternalProject, like several other modules, would probably
benefit on using the C++ implementation instead of having their own
parsing, or using the cmake implementation, but that's for a following
patch.

The ExternalProject module does not use cmake_parse_arguments.
Its argument parsing is a lightweight state machine and mostly just
stores values in target properties for later query. Having to route
that through another argument parsing command would not help much.
All the values would come back in variables just to turn around and
put them in target properties.

-Brad

[Brad King]

Post by Daniele E. Domenichelli
I fixed these issues and merged in next again.

The RunCMake.CMakeParseArguments test fails on the continuous builds.
Please take a look.

Alternatively we can revert this topic pending the proposed C++
re-implementation approach.

Thanks,
-Brad

[Daniele E. Domenichelli]

Post by Brad King
The RunCMake.CMakeParseArguments test fails on the continuous builds.
Please take a look.

Should be fixed now.


Daniele

[Matthew Woehlke]

Post by Brad King
I'd like to avoid new uses of define_property if possible. It
is a legacy from the old builtin documentation system.

Sorry to go off on a tangent, but this triggered a question in my mind...

I have a wrapping utility file that declares some (CMake) helper
functions/macros to automatically generate Python bindings for a C++
library. There is some dependency logic that uses custom target
properties under the hood.

Right now I am using define_property to declare those. Should I (can I)
not do that?

Loosely related: is there a way to make export() and install(EXPORT) set
these properties for exported targets?

[Brad King]

Post by Matthew Woehlke
There is some dependency logic that uses custom target
properties under the hood.
Right now I am using define_property to declare those. Should I (can I)
not do that?

I don't think there is any reason to use define_property except to
mark a directory property as INHERITED into subdirectories. IIRC the
command was created in case we wanted to document properties in CMake
module code instead of builtin C++ DefineProperty calls. That never
really happened and with the new documentation system it is never
needed.

Post by Matthew Woehlke
Loosely related: is there a way to make export() and install(EXPORT) set
these properties for exported targets?

Not of which I'm aware. You can have your package configuration file
add them after loading the imported targets, but that would be a
(centralized) workaround.

-Brad

[Matthew Woehlke]

Post by Brad King

Post by Matthew Woehlke
Loosely related: is there a way to make export() and install(EXPORT) set
these properties for exported targets?

Not of which I'm aware. You can have your package configuration file
add them after loading the imported targets, but that would be a
(centralized) workaround.

For now I probably just won't support importing wrapped libraries.

This would probably be a useful feature to add. I can see it being
useful in any case where a project is sort-of building their own
language/bindings/etc. on top of CMake (e.g. UseJava could possibly
benefit also, to name at least one other).