replace deprecated calls to FetchContent_Populate

[Avus-c]

Fixes: #568

The single argument signature for FetchContent_Populate is deprecated with CMake 3.30.
It was used in order to call add_subdirectory manually with the EXCLUDE_FROM_ALL and SYSTEM flags.
These have been added to FetchContent_Declare with 3.25 and 3.28.

Calling FetchContent_MakeAvailable will then internally call add_subdirectory with EXCLUDE_FROM_ALL and SYSTEM. There is therefore no need to call this manually anymore.

This discussion explains the problem better: https://discourse.cmake.org/t/rfc-deprecating-direct-calls-to-fetchcontent-populate/8161

Have tested this on a few internal projects, and didn't notice any issues.

[Curve]

This does not seem to properly forward OPTIONS passed to CPMFindPackage, works with latest official build but is broken with this patch

Specifically due to this line: https://github.com/cpm-cmake/CPM.cmake/pull/570/files#diff-6fcfee7f313f15253f88285a499e62cb58746d47ff2cfec173f1f4cd29feb44dR888 ${CPM_ARGS_OPTIONS} are no longer set (they were previously set within cpm_add_subdirectory)

[Curve]

@TheLartians Sorry for the ping, but could we get a new release with this PR merged? The original issue's been open for quite some time now and it would greatly reduce the warning spam on our projects at work (and we could move away from using a patched CPM script) :)

[TheLartians]

Hey, thanks for adding this much-needed fix! I agree that we should get this merged asap. I have some small points mostly regarding testing to ensure that this does not break anything, but otherwise this seems good to go.

• As we now have more version specific statements in the code, could we change the our GitHub test workflow matrix to also include a more recent CMake version, e.g. 3.30.0, where FetchContent_Populate is deprecated?
• It seems the examples are now failing due to an issue with EnTT. Can you take a look why it's failing? Perhaps it is already resolved in a more recent version of the library.
• The style pipeline is failing too. Can you make sure to run cmake-format and push again?

[Avus-c]

Still looking into it. There are some issues I don't fully understand yet, when adding a local package and when passing the SOURCE_SUBDIR flag.
Got some fixes locally but the code looks horrendous. :D

[Avus-c]

Ok... this took a bit longer than expected, the devil is in the details...
Anyway, in addition to EXCLUDE and SYSTEM, SOURCE_SUBDIR also needs to be specified in FetchContent_Declare.

The DOWNLOAD_ONLY option is also affected by this change. Unfortunately, I couldn't find an elegant solution for it. *_MakeAvailable will implicitly call add_subdirectory (if a CMakeLists.txt is present in the root folder of the dependency), which is not what we want. This is why the EnTT example failed to build.

The only workaround I found was to call *_Populate and specify exactly where to put the source. This approach essentially replicates the old code-path but uses a function signature that isn't deprecated.
I also had to pass GIT_REPOSITORY,... and other arguments otherwise CMake would throw errors that these are required. I expected that the earlier *_Declare call would be sufficient, but apparently it is ignored in this case.

I'd like to hear your feedback on the DOWNLOAD_ONLY issue.

I don't like it :D, but I haven't found another way of doing it.

Edit: Oh and if you want me to clean up the history a bit (now that half of it are fix commits) just let me know. :D
Edit2: CMake-docs have something about it: Populating Content Without Adding It To The Build
But it's not really helpfull. Basically says, If you only want to download content, call MakeAvailable before calling project().

[Crayon2000]

I'm a bit new to cmake, but the only way I found to use something similar to the DOWNLOAD_ONLY option was to use ExternalProject_Add instead of FetchContent_Declare + FetchContent_MakeAvailable.

Example:

include(ExternalProject)
ExternalProject_Add(
  my_lib
  GIT_REPOSITORY https://github.com/bob/lib.git
  GIT_TAG v1.0.0
  CONFIGURE_COMMAND ""
  BUILD_COMMAND ""
  INSTALL_COMMAND ""
)
ExternalProject_Get_Property(my_lib SOURCE_DIR)
set(my_lib_SOURCE_DIR ${SOURCE_DIR})

[Avus-c]

@Crayon2000 We can't use ExternalProject_Add here. ExternalProject_Add only runs during the build process (including the download, configuration, etc.). You cannot reference libraries, variables, or other elements defined in the dependencies' CMakeLists during the configuration phase of the main project.

This is why FetchContent is preferred in almost all use cases.

If we add add_executable(main main.cpp) and target_link_library(main PRIVATE my_lib) to your example, CMake would fail during the configuration with an error complaining library my_lib is not defined (or something similar).

[TheLartians]

Thanks a lot for the updates! In general this looks good and if it is what we need I'm happy to merge. I only wonder now if there is a way that works on both old and new so we can avoid any version-specific code and behaviour.

Oh and if you want me to clean up the history a bit (now that half of it are fix commits) just let me know. :D

All good, we will squash all commits before merging anyways, so don't worry about the local history here. ;-)

[TheLartians]

Question out of curiosity as I now realise this form of FetchContent_Populate isn't deprecated: could we not only change the original FetchContent_Populate here to the explicit syntax here and have it work on both old and new version of CMake without the version-specific changes? Or is there something I'm missing?

[Avus-c]

Yes, we could switch fully over to *_Populate.

I’m not really a fan of that, since it seems like the CMake devs are pushing us towards using *_Declare and *_MakeAvailable over the last few updates. Switching to *_Populate feels like we’re going against that trend.

That being said, maintaining three different code paths to achieve the same goal is probably alot worse. It complicates the review process and makes the code harder to manage.

I guess, if we run into issues with *_Populate in the future, we can deal with them then. :D

Anyway, latest commit switches everything to *_Populate with the explicit syntax. This has the sideeffect that cpm_declare_fetch could be removed, because it's not being used by anything anymore. And compared to master the changes are minimal.

The unittests passed locally, but I'm out of time and not able to test on some of my larger projects. :D

[Curve]

In the CMake discussion it seemed like any populate calls with explicit arguments are not planned to be deprecated or removed any time soon Maybe we can make use of that to fix this issue with minimal changes and without version specific code?

Edit: Just saw that this is how it's done now - Just want to mention that it doesn't seem like the folks over at CMake are moving away from populate, only the default call is what's mentioned in the discussion, from what I can tell they also need the populate version with explicit params, so I'd encourage going down this route as - if we can trust what has been said in the cmake posts - it is the least intrusive way of fixing this and also future proof

[Avus-c]

Just figured out that when we rely on *_Populate only, we introduce a regression.

FIND_PACKAGE_ARGS and OVERRIDE_FIND_PACKAGE are not supported by FetchContent_Populate.
I use FIND_PACKAGE_ARGS regularly.

Furthermore, I realized that DOWNLOAD_ONLY and FIND_PACKAGE_ARGS are incompatible with each other starting with CMake 3.30. DOWNLOAD_ONLY requires us to use the full *_Populate signature, while FIND_PACKAGE_ARGS is only supported with *_Declare.

This call for example is not valid anymore: (just as an example, not sure if Eigen is actually called libEigen by any package manager)

CPMAddPackage(
  NAME Eigen
  VERSION 3.2.8
  URL https://gitlab.com/libeigen/eigen/-/archive/3.2.8/eigen-3.2.8.tar.gz
  # Eigen's CMakelists are not intended for library use
  DOWNLOAD_ONLY YES 
  FIND_PACKAGE_ARGS NAME libEigen
)

As it stands, FIND_PACKAGE_ARGS does nothing. Reverting this branch back to 6f2cf67 fixes this regression.
However, you still cannot use both at the same time.

Whoops, didn't realize we call find_package manually..... ignore my rambling :D

[TheLartians]

Awesome, thanks for the update! If this works I think it's the best solution as we no longer need to maintain multiple different versions of populating a dependency.

However CI is now failing with

  CMake Error at /opt/hostedtoolcache/cmake/3.16.3/x64/cmake-3.16.3-Linux-x86_64/share/cmake-3.16/Modules/FetchContent.cmake:1055 (add_subdirectory):
    The binary directory
  
      /home/runner/work/CPM.cmake/CPM.cmake/build/integration/fetch_content_compatibility/add_dependency_cpm_and_fetchcontent-bin/_deps/testpack-adder-build
  
    is already used to build a source directory.  It cannot be used to build
    source directory
  
      /home/runner/work/CPM.cmake/CPM.cmake/build/integration/fetch_content_compatibility/add_dependency_cpm_and_fetchcontent-bin/_deps/testpack-adder-src

It's not immediately obvious to me what is responsible for the error, could it be we are specifying the SOURCE_DIR twice through ARGN?

[Avus-c]

@TheLartians

It's not immediately obvious to me what is responsible for the error, could it be we are specifying the SOURCE_DIR twice through ARGN?

No, it seems the problem is that when a dependency is added via *_Populate, it isn't cached or saved internally. When you subsequently call *_Declare and *_MakeAvailable on the same dependency, as shown in this test, *_MakeAvailable tries to add the same source directory again, causing the error. The CMake documentation states that *_Populate is an isolated call. This might be what they mean by that.

Unfortunately, I can't think of a way to work around this problem that doesn't end up at something similar to what we had before (6f2cf67).

[TheLartians]

Unfortunately, I can't think of a way to work around this problem that doesn't end up at something similar to what we had before (6f2cf67).

What's strange to me is that it seemed to have worked before, so I wonder why it fails now. Unless CMake changed the behaviour of this in 3.28.

[Avus-c]

We previously only used *_Populate(name) (single argument), and now *_Populate(name, subdir, ...) (multi argument). I think we have to see these as completely different functions, despite having the same name.
When using *_Populate(name) it was necessary to call *_Declare beforehand. This however is not required and ignored when calling *_Populate(name, subdir, ...).
It must have worked previously because we always had to call *_Declare first, which added the dependency name to some internal "registry" (for a lack of a better term). When this test called *_Declare seperatly it must have been ignored.

[TheLartians]

@Avus-c thanks for the explanation! Given the unexpected complexity of this switch and the relative urgency, I suggest we rollback to 6f2cf67, as it satisfied all test cases at that point. Perhaps we can drop support for legacy CMake versions at some point and loose the version-dependent behaviour at some point in the future.

[Avus-c]

@TheLartians Reverted the commit that attempted to use FetchContent_Populate (multi args) only, as suggested.

I thought about it over the weekend and couldn't come up with any alternative solutions.

I would have liked it if my first contribution had gone a bit smoother :D. But atleast it's working now.

[TheLartians]

Thanks again for this much needed fix!

Sorry for all the change requests, the FetchContent_Populate syntax change turned out to have much more side-effects than I expected making it a non-trivial replacement.

Really happy we have it working now and tested from 3.16 to 3.30!

[TheLartians]

The fix is now live in v0.40.1 🎉