Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserving forwarding of empty string arguments #461

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

CraigHutchinson
Copy link
Contributor

@CraigHutchinson CraigHutchinson commented Mar 7, 2023

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 CraigHutchinson force-pushed the preserve_empty_strings branch from 43f9f96 to 477d827 Compare March 7, 2023 15:48
@CraigHutchinson
Copy link
Contributor Author

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 CraigHutchinson force-pushed the preserve_empty_strings branch 2 times, most recently from 98782df to be08173 Compare March 8, 2023 10:11
@CraigHutchinson
Copy link
Contributor Author

CraigHutchinson commented Mar 8, 2023

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
Copy link
Member

TheLartians commented Mar 19, 2023

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

Copy link
Member

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

CraigHutchinson commented Mar 20, 2023

  • 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 Outdated Show resolved Hide resolved
@CraigHutchinson
Copy link
Contributor Author

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

@CraigHutchinson
Copy link
Contributor Author

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

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

CraigHutchinson commented Mar 23, 2023

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
Copy link
Member

TheLartians commented Apr 13, 2023

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

@CraigHutchinson
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

CraigHutchinson commented Aug 9, 2023

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
Copy link
Member

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
Copy link
Contributor

ClausKlein commented Aug 19, 2023

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
Copy link
Contributor

What is about this MR?

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

Comment on lines +519 to +533
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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

jdumas commented Mar 20, 2024

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
Copy link
Member

TheLartians commented Apr 7, 2024

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.

@jdumas
Copy link

jdumas commented Dec 6, 2024

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

@ClausKlein
Copy link
Contributor

ClausKlein commented Dec 6, 2024

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)

@jdumas
Copy link

jdumas commented Dec 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants