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

Weird issue with CPM_SOURCE_CACHE and FetchContent #281

Closed
mrexodia opened this issue Jul 27, 2021 · 10 comments · Fixed by #352
Closed

Weird issue with CPM_SOURCE_CACHE and FetchContent #281

mrexodia opened this issue Jul 27, 2021 · 10 comments · Fixed by #352

Comments

@mrexodia
Copy link

mrexodia commented Jul 27, 2021

In the top-level project there is https://github.com/vtil-project/VTIL-Utils/blob/master/cmake/VTIL-Core.cmake:

CPMAddPackage(
    NAME VTIL-Core
    GIT_REPOSITORY https://github.com/mrexodia/VTIL-Core
    GIT_TAG e6debb384cb218caff8cb94f50f00076b994a762
)

And after that it includes https://github.com/vtil-project/VTIL-Utils/blob/master/cmake/VTIL-NativeLifters.cmake:

CPMAddPackage(
    NAME VTIL-NativeLifters
    GIT_REPOSITORY https://github.com/vtil-project/VTIL-NativeLifters
    GIT_TAG f8837ef4d2412a5a1c25f725f25685a23e1cf1a1
)

Inside the VTIL-NativeLifters repo there is a FetchContent call with the name VTIL-Core, The idea here is that you can pin a different version of VTIL-Core in your project that will then be used to compile the VTIL-NativeLifters library.

https://github.com/vtil-project/VTIL-NativeLifters/blob/master/Dependencies/CMakeLists.txt#L4:

include(FetchContent)

message(STATUS "Fetching VTIL-Core (this might take a while)...")
FetchContent_Declare(
    VTIL-Core
    GIT_REPOSITORY https://github.com/vtil-project/VTIL-Core
    GIT_TAG        eeffb26b8b9ef7b4bd6ffbfc09f3133b70b36eda
    GIT_SHALLOW    false
)
FetchContent_MakeAvailable(VTIL-Core)

When using CPM_SOURCE_CACHE CMake errors out:

D:\CodeBlocks\VTIL-Utils\build>cmake ..
-- Building for: Visual Studio 16 2019
-- Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.19042.
-- The C compiler identification is MSVC 19.29.30038.1
-- The CXX compiler identification is MSVC 19.29.30038.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30037/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30037/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Fetching capstone (this might take a while)...
CMake Deprecation Warning at build/_deps/capstone-src/CMakeLists.txt:1 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.


CMake Deprecation Warning at build/_deps/capstone-src/CMakeLists.txt:18 (cmake_policy):
  The OLD behavior for policy CMP0048 will be removed from a future version
  of CMake.

  The cmake-policies(7) manual explains that the OLD behaviors of all
  policies are deprecated and that a policy should be set to OLD only under
  specific short-term circumstances.  Projects should be ported to the NEW
  behavior and not rely on setting a policy to OLD.


Enabling CAPSTONE_ARM_SUPPORT
Enabling CAPSTONE_ARM64_SUPPORT
Enabling CAPSTONE_M68K_SUPPORT
Enabling CAPSTONE_MIPS_SUPPORT
Enabling CAPSTONE_PPC_SUPPORT
Enabling CAPSTONE_SPARC_SUPPORT
Enabling CAPSTONE_SYSZ_SUPPORT
Enabling CAPSTONE_XCORE_SUPPORT
Enabling CAPSTONE_X86_SUPPORT
Enabling CAPSTONE_TMS320C64X_SUPPORT
Enabling CAPSTONE_M680X_SUPPORT
Enabling CAPSTONE_EVM_SUPPORT
Enabling CAPSTONE_MOS65XX_SUPPORT
-- Fetching keystone (this might take a while)...
-- Looking for pthread.h
-- Looking for pthread.h - not found
-- Found Threads: TRUE
-- CPM: adding package VTIL-Core@0 (e6debb384cb218caff8cb94f50f00076b994a762 at d:/cpm_cache/vtil-core/b00df6cc2b142d6fa5bce7908fa7339da177c741)
-- Fetching VTIL-Core (this might take a while)...
CMake Error at C:/Program Files/CMake/share/cmake-3.20/Modules/FetchContent.cmake:1206 (add_subdirectory):
  The binary directory

    D:/CodeBlocks/VTIL-Utils/build/_deps/vtil-core-build

  is already used to build a source directory.  It cannot be used to build
  source directory

    D:/CodeBlocks/VTIL-Utils/build/_deps/vtil-core-src

  Specify a unique binary directory name.
Call Stack (most recent call first):
  D:/cpm_cache/vtil-nativelifters/b0b7a3915a18707147fb220d4cf2d2f76e9d8d8d/Dependencies/CMakeLists.txt:10 (FetchContent_MakeAvailable)


-- CPM: adding package VTIL-NativeLifters@0 (f8837ef4d2412a5a1c25f725f25685a23e1cf1a1 at d:/cpm_cache/vtil-nativelifters/b0b7a3915a18707147fb220d4cf2d2f76e9d8d8d)
-- CPM: adding package args@0 (ae22269df734a2b0957a9ab4e37be41f61866dbe at d:/cpm_cache/args/0fef5c19d6fb87e7b5acefd2362b022afaf2dab6)
-- CPM: adding package CPMLicenses.cmake@0.0.5 (v0.0.5 at d:/cpm_cache/cpmlicenses.cmake/2e9893c848f8cf158d41d1c4737a276bac779ef3)
-- Configuring incomplete, errors occurred!
See also "D:/CodeBlocks/VTIL-Utils/build/CMakeFiles/CMakeOutput.log".
See also "D:/CodeBlocks/VTIL-Utils/build/CMakeFiles/CMakeError.log".

Without CPM_SOURCE_CACHE everything works as intended:

D:\CodeBlocks\VTIL-Utils\build>cmake ..
-- Building for: Visual Studio 16 2019
-- Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.19042.
-- The C compiler identification is MSVC 19.29.30038.1
-- The CXX compiler identification is MSVC 19.29.30038.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30037/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30037/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- CPM: adding package VTIL-Core@0 (e6debb384cb218caff8cb94f50f00076b994a762)
-- Fetching capstone (this might take a while)...
CMake Deprecation Warning at build/_deps/capstone-src/CMakeLists.txt:1 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.


CMake Deprecation Warning at build/_deps/capstone-src/CMakeLists.txt:18 (cmake_policy):
  The OLD behavior for policy CMP0048 will be removed from a future version
  of CMake.

  The cmake-policies(7) manual explains that the OLD behaviors of all
  policies are deprecated and that a policy should be set to OLD only under
  specific short-term circumstances.  Projects should be ported to the NEW
  behavior and not rely on setting a policy to OLD.


Enabling CAPSTONE_ARM_SUPPORT
Enabling CAPSTONE_ARM64_SUPPORT
Enabling CAPSTONE_M68K_SUPPORT
Enabling CAPSTONE_MIPS_SUPPORT
Enabling CAPSTONE_PPC_SUPPORT
Enabling CAPSTONE_SPARC_SUPPORT
Enabling CAPSTONE_SYSZ_SUPPORT
Enabling CAPSTONE_XCORE_SUPPORT
Enabling CAPSTONE_X86_SUPPORT
Enabling CAPSTONE_TMS320C64X_SUPPORT
Enabling CAPSTONE_M680X_SUPPORT
Enabling CAPSTONE_EVM_SUPPORT
Enabling CAPSTONE_MOS65XX_SUPPORT
-- Fetching keystone (this might take a while)...
-- Looking for pthread.h
-- Looking for pthread.h - not found
-- Found Threads: TRUE
-- CPM: adding package VTIL-NativeLifters@0 (f8837ef4d2412a5a1c25f725f25685a23e1cf1a1)
-- Fetching VTIL-Core (this might take a while)...
-- CPM: adding package args@0 (ae22269df734a2b0957a9ab4e37be41f61866dbe)
-- CPM: adding package CPMLicenses.cmake@0.0.5 (v0.0.5)
-- Configuring done
-- Generating done
-- Build files have been written to: D:/CodeBlocks/VTIL-Utils/build

This is cmake 3.20.5 btw

@mrexodia
Copy link
Author

mrexodia commented Jul 27, 2021

My current guess is that there is no call to FetchContent_Declare when cached, so likely these functions don't know there is already content for VTIL-Core? https://gitlab.kitware.com/cmake/cmake/-/blob/f7cf69e34a1607e8ea2b6d10fef6a6058377c24e/Modules/FetchContent.cmake#L651-700

@TheLartians
Copy link
Member

Hey, thanks for the issue!

I agree that the problem seems to be caused by FetchContent not being aware of the existence of VTIL-Core, resulting in the dependency being added twice to the project. Currently, you could probably fix the issue by disabling caching for that specific dependency, by adding the argument NO_CACHE true to the CPMAddPackage call of VTIL-Core.

I think it might make sense for CPM.cmake to add another parameter that still allows caching while using FetchContent to declare packages, to support such use-cases.

@mrexodia
Copy link
Author

For now I indeed disabled CPM caching, but I think this should be considered a bug (the behavior for cache vs no cache should be the same I think). Would it be possible to always use FetchContent (eg point to the cache directories directly instead of downloading)?

@TheLartians
Copy link
Member

I agree that it would be nice to have FetchContent and CPM.cmake play nicely together in all circumstances. The reason we have ditched FetchContent where possible was the drastic performance impact that it brings (sometimes on the order of seconds per dependency when configuring). As projects can easily have hundreds of dependencies, this isn't exactly scalable, so we now use FetchContent only for the initial download.

However, I haven't yet checked if we are able to use FetchContent_Declare without running into the performance hit. If so, it would definitely make sense to declare the packages with the correct source directory, as you suggested.

mrexodia referenced this issue Sep 3, 2021
…ge (#288)

Add fetchcontent_dependency unit test:
* This test should highlight the fact that cpm_add_subdirectory is always called, even when
  cpm_fetch_package isn't populating the dependency
* NO_CACHE YES highlight a bug introduced in 32b063e where
  cpm_fetch_package was checking undefined ${lower_case_name}_POPULATED variable

#287
@jm4R
Copy link

jm4R commented Dec 29, 2021

It seems this bug gets to many people and probably should be considered as critical. I am one of them - just found the CPM (I think it's a brilliant idea) and tried to use it in my projects and my first probes gets me to this. I guess I may be one of many. Disabling the cache is not a good workaround as it removes 90% of this module benefits.

@DNKpp
Copy link
Contributor

DNKpp commented Dec 31, 2021

@jm4R is still present in the current Version?

@jm4R
Copy link

jm4R commented Jan 3, 2022

@DNKpp yes, but I am not sure if it is exact the same use case. I will describe you mine:

my_project
├── fmtlib
└── labA
    └── fmtlib

Where libA fetches exactly the same version of fmtlib but using classic FetchContent

After fixing that one by overriding binary directory of direct dependency, another issue appeared:

my_project
├── fmtlib (1)
├── libA
│   └── fmtlib (2)
└── libB
    └── fmtlib (3)

This time there is a conflict between (2) and (3). There were no such conflict when libA and libB was fetched by classic FetchContent


My guess to fix this would be to override this defaults by some CPM-defaults:

The SOURCE_DIR and BINARY_DIR arguments are supported by ExternalProject_Add(), but different default values are used by FetchContent_Populate(). SOURCE_DIR defaults to ${CMAKE_CURRENT_BINARY_DIR}/-src and BINARY_DIR defaults to ${CMAKE_CURRENT_BINARY_DIR}/-build. If a relative path is specified, it will be interpreted as relative to CMAKE_CURRENT_BINARY_DIR.

For example by replacing ${CMAKE_CURRENT_BINARY_DIR}/<lowercaseName>-build with ${CMAKE_CURRENT_BINARY_DIR}/<lowercaseName>-cpmbuild. Also it could be an improvement to just use cache path directly for reading source instead of copying it to ${CMAKE_CURRENT_BINARY_DIR}/<lowercaseName>-src

@lhamowski
Copy link

I have the same problem as jm4r mentioned.

TheLartians added a commit that referenced this issue May 16, 2022
* Override FetchContent paramers, fixes #281

* add check to verify that dependency is cached

* update test comment

* rename test file and class

* update test indentation
@TheLartians
Copy link
Member

FYI: CPM should now be able to override FetchContent calls since v0.35.1! Please me know if you encounter further issues with this.

@jm4R
Copy link

jm4R commented May 19, 2023

@DNKpp @TheLartians Just revisited this issue: Problem easily reproduced with 0.38.1 :(

Workaround is wrapping a CPMAddPackage wit a function:

function(add_dependency depname)
    FetchContent_GetProperties(${depname})
    string(TOLOWER ${depname} depname_lc)
    if(NOT "${${depname_lc}_POPULATED}")
        CPMAddPackage(NAME ${depname} ${ARGV})
    endif()
endfunction()

And call it:

add_dependency(fmt
    URL      https://github.com/fmtlib/fmt/releases/download/7.0.3/fmt-7.0.3.zip
    URL_HASH SHA256=decfdf9ad274070fa85f26407b816f5a4d82205ae86bac1990be658d0795ea4d
)

BTW: is there a way to revert to the behavior that cached dependencies are copied to <build>/_deps? Can be helpful when preparing reproducible builds.

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 a pull request may close this issue.

5 participants