So the code using EVAL is CMake 3.18, this is currently failing. There is a legacy functionality in the docs but not sure it will handle empty strings. https://cmake.org/cmake/help/latest/command/cmake_language.html#evaluating-code

Potential resolution is to fix <3.18 by passing arguments the normal way and maybe warn when empty-values are provided as arguments as not-supported etc. TBC

CI Integration is failing due to. @TheLartians is this something CI related or some side-effect of this change?

/home/runner/work/CPM.cmake/CPM.cmake/build/integration/system_warnings/dependency_added_using_system-system-bin/_deps/adder-src/code/adder/adder.hpp:4:38:
 error: no return statement in function returning non-void [-Werror=return-type]
 4 | inline int emitWarning(int unused) { }

@TheLartians is this something CI related or some side-effect of this change?

From the diff of the PR it seems the SYSTEM option was removed in two places (possibly a merge issue) which would cause CI to fail.

• IMO the solution replacing the whole CPMAddPackage call with something using eval looks complicated and potentially risky for such a niche issue. Is there an easier way to handle it? E.g. in here it was recommended to simply point the flag to a non-submodule directory instead.

20579 is unfortunately just a hack which doesn't fit with user expectations imho. CMake applied the fix to that in https://gitlab.kitware.com/cmake/cmake/-/merge_requests/4729 which resolved this behavior for FetchContent>ExternalProject (among others) as the best UX option available. This PR mirrors the fix into CPM so that irrespective of there being new or other potential empty-string parameters they will all be correctly forwarded to the lower functions as the user expects.

Internally using EVAL feels somewhat hacky but it appears to be the defacto workaround given the language limitations from what I can see!

• Could you add a test case to check that it works as intended?

💯 I will be updating and provide additional tests soon.

I currently have a 'minor' concern over the styling rules. Ideally I would say to prefer putting each Key-value pair on a separate line when their are multiple options. Justification is that this increases readability and especially change-diff is much more readable. I wonder if this is an option?...
image

I applied the same fix as is used under package-lock_prettify.cmake to disable cmake-format on the offending lines

Unit test added - Fails pre-changes and all passing post-fixes (Shall note that CPMDeclarePackage always preserved arguments but added to increase test coverage). To check the internals specific functions are stubbed. This appears the only way to introspect FetchContent_Declare and others. It does feel like there may be other areas of CPM should ensure argument list is maintained that could be covered in future tests. The new tests focus here to cover the areas improved by this PR primarily.

Thanks for adding the test case! What do you think about using placeholder arguments instead of eval as suggested here?

Thanks for adding the test case! What do you think about using placeholder arguments instead of eval as suggested here?

That sounds great, I will take a deeper look really soon. It would be nice to not need Eval, albeit that is was used internally for CMake it doesn't mean it's the best and only option.

What is the state of this MR?

We have problems to prevent the cloning of git submodules using this CMP0097 CMake policy.

i.e.:

# NOTE(CK): Set GIT_SUBMODULES to empty string to NOT initializes submodules!
cmake_policy(SET CMP0097 NEW)
CPMAddPackage(
 NAME project_options
 GIT_TAG v0.30.0
 GITHUB_REPOSITORY aminya/project_options
 # FIXME: GIT_SUBMODULES ""
 GIT_SUBMODULES "docs"
 # NOTE(CK): Workaround existing dir to prevent load of submodule
)

Thanks for adding the test case! What do you think about using placeholder arguments instead of eval as suggested here?

@TheLartians Appologies I have changed roles and not using CMake for a while ( 😢 ) and forgot to review the alternative approach. I did have a look and wasn't quite as keen on it as the implementation needed somewhat more time to decipher so not sure its as easy to maintain.

May I propose that we use the approach wihtin this PR to unblock @ClausKlein and the non-eval alternative implementation could be reviewed as a separate PR improving this feature?

I may be a bit too paranoid, but using a mechanism like eval for something simple like argument forwarding leaves me with a bad feeling. In my suggestion we essentially replace empty arguments using a placeholder string and remove it right before forwarding them.

I agree that it does add a bit more code and complexity than your version, but with proper docstrings and tests cases I hope that maintainability shouldn't be too big of an issue for this.

No!

ExternalProject
and
FetchContent

have a lot of parameters and with just every CMake version, there will be more ...

CMP.cmake is going to get a miss designed interface for _UNPARSED_ARGUMENTS

I.e.: SYSTEM ON; EXCLUDE_FROM_ALL NO; ... ; OVERRIDE_FIND_PACKAGE ?

What is about this MR?

Perhaps this helps to find a solution: forwarding-command-arguments-in-cmake

Hi. Have we reached a consensus regarding this PR yet? It seems we have either this version or this alternative as a possibility? Personally if the upstream FetchContent module is using cmake_language(EVAL ...) I don't see why we couldn't do the same in CPM.

Without this feature it's currently impossible to disable cloning submodules with CPM, which can add quite a bit of extra things to download during project configuration.

Yeah, I agree that going with the eval mechanism for now is ok, considering it is blocking an important feature. I still want to double-check the PR before, but unfortunately I do not have a lot of time at the moment.

Hi @TheLartians . I was wondering if there's any update on potentially merging this feature?

Without this feature it's currently impossible to disable cloning submodules with CPM, which can add quite a bit of extra things to download during project configuration.

There is a workaround : set an existing subdirectory to disable recursive cloning.
see #461 (comment)

Oh nice, I missed this! I'll try that workaround, thanks!