Preserving forwarding of empty string arguments #461

CraigHutchinson · 2023-03-07T15:44:23Z

Background

This PR solves CPM issue #460

Empty-string arguments are not handled by Cmake well. There is a workaround here that is a hack for one specific parameter GIT_SUBMODULES https://gitlab.kitware.com/cmake/cmake/-/issues/20579 but there is also a proper fix to forwarding empty-string arguments applied under https://gitlab.kitware.com/cmake/cmake/-/merge_requests/4729 which closed the Issue. CPM needs the same/similar also.

This PR

This PR mirrors the changes in https://gitlab.kitware.com/cmake/cmake/-/merge_requests/4729 so that CPM will forward all arguments onto FetchContent>ExternalProject unmodified when they contain empty string arguments

Changes

use cmake_parse_arguments(PARSE_ARGV which supports Empty-string arguments e.g. key-value pairs such as GIT_SUBMODULES ""
Use cmake_language(EVAL for perfect-forwarding of empty-string argument lists

Also:

Support cmake-format on Windows
Auto fixes some whitespace

Separate concern

For separate PR (future):

CPMFindPackage not updated

ToDO

Fix for pre 3.18 usage of EVAL
Investigate/fix Test package-lock_prettify TYPO-Fixed
Add unit-test for empty string argument e.g. GIT_SUBMODULES ""

CraigHutchinson · 2023-03-07T20:46:52Z

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

CraigHutchinson · 2023-03-08T10:32:27Z

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 · 2023-03-19T21:07:15Z

@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.

TheLartians

I'm having a hard time understanding this PR. To be sure, the issue is that it is currently impossible to disable recursive cloning using FetchContent?

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.
Could you add a test case to check that it works as intended?

CraigHutchinson · 2023-03-20T08:43:19Z

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.

cmake/CPM.cmake

CraigHutchinson · 2023-03-21T09:22:14Z

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?...

CraigHutchinson · 2023-03-21T09:38:31Z

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

Also: - Support cmake-format on Windows by using auto lione-ending

CraigHutchinson · 2023-03-23T21:00:55Z

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.

TheLartians · 2023-04-13T16:14:48Z

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

CraigHutchinson · 2023-04-13T16:49:51Z

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.

ClausKlein · 2023-08-04T05:44:45Z

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
)

CraigHutchinson · 2023-08-09T07:58:36Z

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?

TheLartians · 2023-08-19T12:22:16Z

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.

ClausKlein · 2023-08-19T18:08:07Z

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 ?

ClausKlein · 2023-12-08T06:13:25Z

What is about this MR?

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

ClausKlein · 2023-12-11T11:38:36Z

cmake/CPM.cmake

+macro(CPMAddPackage)
+  set(__ARGN "${ARGN}")
+  list(LENGTH __ARGN __ARGN_Length)
+  if(__ARGN_Length EQUAL 1)
+    cpm_add_package_single_arg(${ARGN})
+  else()
+    # Forward preserving empty string arguments
+    # (https://gitlab.kitware.com/cmake/cmake/-/merge_requests/4729)
+    set(__ARGN_Quoted)
+    foreach(__ARG IN LISTS __ARGN)
+      string(APPEND __ARGN_Quoted " [==[${__ARG}]==]")
+    endforeach()
+    cpm_cmake_eval("cpm_add_package_multi_arg( ${__ARGN_Quoted} )")
+  endif()
+endmacro()


There is, however, a specific case to be aware of which can result in unexpected behavior. Because macros treat their arguments as string substitutions rather than as variables, if they use ARGN in a place where a variable name is expected, the variable it will refer to will be in the scope from which the macro is called, not the ARGN from the macro’s own arguments.

From chapter 9.2 professional-cmake

jdumas · 2024-03-20T00:08:28Z

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.

TheLartians · 2024-04-07T21:29:21Z

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.

CraigHutchinson force-pushed the preserve_empty_strings branch from 43f9f96 to 477d827 Compare March 7, 2023 15:48

CraigHutchinson force-pushed the preserve_empty_strings branch 2 times, most recently from 98782df to be08173 Compare March 8, 2023 10:11

TheLartians reviewed Mar 19, 2023

View reviewed changes

CraigHutchinson commented Mar 20, 2023

View reviewed changes

cmake/CPM.cmake Show resolved Hide resolved

CraigHutchinson commented Mar 20, 2023

View reviewed changes

cmake/CPM.cmake Outdated Show resolved Hide resolved

CraigHutchinson force-pushed the preserve_empty_strings branch 2 times, most recently from 303ae81 to f4fd660 Compare March 23, 2023 20:55

CraigHutchinson and others added 5 commits March 23, 2023 20:55

Test forwarding of arguments to FetchCOntent_Declare

b70460a

Preserving forwarding of empty string arguments

cbe5144

Also: - Support cmake-format on Windows by using auto lione-ending

Support EVAL CODE on CMake <3.18

b847af6

Fix docs typo

5c1ce67

Maintain key-value CMake formatting

f4fd660

CraigHutchinson requested a review from TheLartians March 23, 2023 21:10

TheLartians mentioned this pull request Apr 13, 2023

Refactor empty argument passing to not require eval CraigHutchinson/CPM.cmake#1

Open

TheLartians mentioned this pull request Apr 18, 2023

Bug: GIT_SUBMODULES_RECURSE and GIT_SUBMODULES are ignored #467

Closed

Merge branch 'master' into preserve_empty_strings

3e11e3b

Merge branch 'master' into preserve_empty_strings

e6e6524

ClausKlein reviewed Dec 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserving forwarding of empty string arguments #461

Preserving forwarding of empty string arguments #461

CraigHutchinson commented Mar 7, 2023 •

edited

Loading

CraigHutchinson commented Mar 7, 2023

CraigHutchinson commented Mar 8, 2023 •

edited

Loading

TheLartians commented Mar 19, 2023 •

edited

Loading

TheLartians left a comment

CraigHutchinson commented Mar 20, 2023 •

edited

Loading

CraigHutchinson commented Mar 21, 2023

CraigHutchinson commented Mar 21, 2023

CraigHutchinson commented Mar 23, 2023 •

edited

Loading

TheLartians commented Apr 13, 2023 •

edited

Loading

CraigHutchinson commented Apr 13, 2023

ClausKlein commented Aug 4, 2023

CraigHutchinson commented Aug 9, 2023 •

edited

Loading

TheLartians commented Aug 19, 2023

ClausKlein commented Aug 19, 2023 •

edited

Loading

ClausKlein commented Dec 8, 2023

ClausKlein Dec 11, 2023

jdumas commented Mar 20, 2024

TheLartians commented Apr 7, 2024 •

edited

Loading

Preserving forwarding of empty string arguments #461

Are you sure you want to change the base?

Preserving forwarding of empty string arguments #461

Conversation

CraigHutchinson commented Mar 7, 2023 • edited Loading

Background

This PR

Changes

Separate concern

ToDO

CraigHutchinson commented Mar 7, 2023

CraigHutchinson commented Mar 8, 2023 • edited Loading

TheLartians commented Mar 19, 2023 • edited Loading

TheLartians left a comment

Choose a reason for hiding this comment

CraigHutchinson commented Mar 20, 2023 • edited Loading

CraigHutchinson commented Mar 21, 2023

CraigHutchinson commented Mar 21, 2023

CraigHutchinson commented Mar 23, 2023 • edited Loading

TheLartians commented Apr 13, 2023 • edited Loading

CraigHutchinson commented Apr 13, 2023

ClausKlein commented Aug 4, 2023

CraigHutchinson commented Aug 9, 2023 • edited Loading

TheLartians commented Aug 19, 2023

ClausKlein commented Aug 19, 2023 • edited Loading

No!

ClausKlein commented Dec 8, 2023

ClausKlein Dec 11, 2023

Choose a reason for hiding this comment

jdumas commented Mar 20, 2024

TheLartians commented Apr 7, 2024 • edited Loading

CraigHutchinson commented Mar 7, 2023 •

edited

Loading

CraigHutchinson commented Mar 8, 2023 •

edited

Loading

TheLartians commented Mar 19, 2023 •

edited

Loading

CraigHutchinson commented Mar 20, 2023 •

edited

Loading

CraigHutchinson commented Mar 23, 2023 •

edited

Loading

TheLartians commented Apr 13, 2023 •

edited

Loading

CraigHutchinson commented Aug 9, 2023 •

edited

Loading

ClausKlein commented Aug 19, 2023 •

edited

Loading

TheLartians commented Apr 7, 2024 •

edited

Loading