Fetch content #446
Fetch content #446
Conversation
|
Concerning the Style it seems there is a clash with my Clion and the cmake-format file in the repo. The file gets regenerated with the wrong formatting. Maybe a PR with a pre-commit config files would help |
CMakeLists.txt
Outdated
| LANGUAGES NONE | ||
| ) | ||
|
|
||
| option(CPM_INSTALL "Install CPM" OFF) |
There was a problem hiding this comment.
Is the option necessary? Maybe it is to avoid install on ExternalProject, but if that's the case better use the if(CMAKE_PROJECT_NAME ...)
There was a problem hiding this comment.
I was thinking about people using CPM just to compile their project, forcing install would be unnecessary. But for a framework it could be interesting to allow the install of CPM
There was a problem hiding this comment.
That is not necessary. Cmake only installs if you pass cmake --install .. The only issue that could occur (I neeed to test this), is that calling cmake --install . on a downstream project would install the upstreams as well. But I haven't tested that, and if it's not the case, then we're all safe.
There was a problem hiding this comment.
I think it is the case, CMake will install all even upstreams
There was a problem hiding this comment.
If you really want to supress this option, maybe we could create a custom target only if we detect CPM is not the main repo and consumer could ask explicitly to install it or make it a dependency to one of the consumer's targets
There was a problem hiding this comment.
Indeed I confirm this is the case (need to fix my packages). Check the review later on about what is a sufficient way to circumvent this option.
CMakeLists.txt
Outdated
| set(CPM_RELEASE FALSE) | ||
| endif() | ||
| endif() | ||
| find_package(CPM CONFIG REQUIRED NO_POLICY_SCOPE) |
There was a problem hiding this comment.
How does this work? I've never thought you could find_package() itself especially without the CPMConfig files being generated. Did you test it on a clean machine?
There was a problem hiding this comment.
The CPMConfig is generated but not installed automatically. I think generally maybe it could not work but in our case we just need to include CPM.cmake so it works. It is working because I have provided CPM_DIR to say where to find the generated Config file
I did some test on my computer. It seems to work
There was a problem hiding this comment.
It seems you can use an Configfile generated inside the same project and CMake propose :
New in version 3.1: If the INSTALL_PREFIX argument is passed, this is used as base path to calculate all the relative paths. The <path> argument must be an absolute path. If this argument is not passed, the [CMAKE_INSTALL_PREFIX](https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html#variable:CMAKE_INSTALL_PREFIX) variable will be used instead. The default value is good when generating a FooConfig.cmake file to use your package from the install tree. When generating a FooConfig.cmake file to use your package from the build tree this option should be used.
but I don't really catch the meaning and how to use it.
There was a problem hiding this comment.
Ok, I see how that works, but this will mark release tarballs as CPM_RELEASE=FALSE.
A more manageable approach is to set CPM_RELEASE=FALSE on main branch. Then to publish a release:
- Branch
maintoX.Y.Zbranch - Rebase a commit that edits
set(CPM_RELEASE OFF)->set(CPM_RELEASE ON) - Tag release to this branch
There was a problem hiding this comment.
To my understanding release tarballs will not have a .git folder inside so the CPM_RELEASE will be unset in this case. I see you point. What you propose would work but this requires CPM maintainers to agree on this behaviour. I tried to stay conservative on the workflows and CI
I think for now this would work :
if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/.git" AND IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/.git")
find_package(Git REQUIRED QUIET)
# Generate a git-describe version string from Git repository tags.
execute_process(
COMMAND ${GIT_EXECUTABLE} tag --points-at HEAD
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
OUTPUT_VARIABLE CPM_TAG
RESULT_VARIABLE GIT_DESCRIBE_ERROR_CODE
OUTPUT_STRIP_TRAILING_WHITESPACE
)
if(NOT GIT_DESCRIBE_ERROR_CODE AND NOT CPM_TAG STREQUAL "")
set(CPM_RELEASE TRUE)
else()
set(CPM_RELEASE FALSE)
endif()
else()
set(CPM_RELEASE TRUE)
endif()There was a problem hiding this comment.
Yes, there is also a possibility of the opposite (downloading a tarball for a specific commit), but let's ignore this for now.
I think we should ping the maintainers about the long-term approach, either here or a separate issue.
There was a problem hiding this comment.
I didn't know you could do this :) but it seems very a niche scenario, that it is not possible for now with get_cpm anyway.
Yeah, I prefer CLion's formatting in some parts as well. I've opened an issue about that on cmake-format. The pre-commit file for that would be straightforward but would need the owner to add pre-commit-ci to automatically cmake-format |
At least having a pre-commit config could simplify the process for developper. It can be easy to forget to run cmake-format. With pre-commit it would be at least automatic on developper computers. pre-commit.ci for the repo would be the best option of course |
CMakeLists.txt
Outdated
| LANGUAGES NONE | ||
| ) | ||
|
|
||
| option(CPM_INSTALL "Install CPM" OFF) |
There was a problem hiding this comment.
Indeed I confirm this is the case (need to fix my packages). Check the review later on about what is a sufficient way to circumvent this option.
Co-authored-by: Cristian Le <[email protected]>
Co-authored-by: Cristian Le <[email protected]>
|
@LecrisUT Does everything looks fine now? |
|
I don't know how to test this case |
|
lgtm Here's a snippet you can run: cmake_minimum_required(VERSION 3.24)
project(TestCPMFetchContent)
include(FetchContent)
FetchContent_Declare(CPM
GIT_REPOSITORY https://github.com/flagarde/CPM.cmake
GIT_TAG FetchContent
OVERRIDE_FIND_PACKAGE)
find_package(CPM) |
|
Hmm my tests on this do not succeed yet. I've added a |
CMakeLists.txt
Outdated
| else() | ||
| set(CPM_RELEASE TRUE) | ||
| endif() | ||
| find_package(CPM CONFIG REQUIRED NO_POLICY_SCOPE) |
There was a problem hiding this comment.
We are over-complicating it here. This works just fine
| find_package(CPM CONFIG REQUIRED NO_POLICY_SCOPE) | |
| include(cmake/CPM.cmake) |
There was a problem hiding this comment.
Ok, I found a small bug with how we are looking for the .git I can change this and your changes.
I tested using :
cmake_minimum_required(VERSION 3.24)
project(TestCPMFetchContent)
include(FetchContent)
FetchContent_Declare(CPM
GIT_REPOSITORY https://github.com/flagarde/CPM.cmake
GIT_TAG FetchContent
OVERRIDE_FIND_PACKAGE)
FetchContent_MakeAvailable(CPM)
find_package(CPM)
# the install option has to be explicitly set to allow installation
CPMAddPackage(
GITHUB_REPOSITORY jarro2783/cxxopts
VERSION 2.2.1
OPTIONS "CXXOPTS_BUILD_EXAMPLES NO" "CXXOPTS_BUILD_TESTS NO" "CXXOPTS_ENABLE_INSTALL YES"
)In this case not loop but a crash :
CMake Error at CMakeLists.txt:15 (CPMAddPackage):
Unknown CMake command "CPMAddPackage".
-- Configuring incomplete, errors occurred!without OVERRIDE_FIND_PACKAGE :
-- The C compiler identification is GNU 12.2.1
-- The CXX compiler identification is GNU 12.2.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - 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: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
fatal : ni ceci ni aucun de ses répertoires parents (jusqu'au point de montage /) n'est un dépôt git
Arrêt à la limite du système de fichiers (GIT_DISCOVERY_ACROSS_FILESYSTEM n'est pas défini).
CMake Warning at CMakeLists.txt:12 (find_package):
By not providing "FindCPM.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "CPM", but
CMake did not find one.
Could not find a package configuration file provided by "CPM" with any of
the following names:
CPMConfig.cmake
cpm-config.cmake
Add the installation prefix of "CPM" to CMAKE_PREFIX_PATH or set "CPM_DIR"
to a directory containing one of the above files. If "CPM" provides a
separate development package or SDK, be sure it has been installed.
-- CPM: Adding package [email protected] (v2.2.1)
-- cxxopts version 2.2.0
-- Configuring done
-- Generating done
There was a problem hiding this comment.
Now it seems to work :)
Please have a try too.. just in case
There was a problem hiding this comment.
All good now. For an initial integration I'm 👍 on this.
CMakeLists.txt
Outdated
| configure_file( | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/cmake/CPM.cmake" "${CMAKE_CURRENT_BINARY_DIR}/CPM.cmake" COPYONLY | ||
| ) | ||
| set(CPM_DIR "${CMAKE_CURRENT_BINARY_DIR}") |
There was a problem hiding this comment.
Not really necessary. In my tests, these are not set upstream. I don't understand how but:
CPM_DIR(set in this file) andCURRENT_CPM_VERSION(set in theinclude(cmake/CPM.cmake)) do not propagate upstream- But we can still use the macro/functions defined in
cmake/CPM.cmake
cmake_minimum_required(VERSION 3.24)
project(TestCPMFetchContent)
include(FetchContent)
FetchContent_Declare(CPM
GIT_REPOSITORY https://github.com/flagarde/CPM.cmake
GIT_TAG FetchContent
)
FetchContent_MakeAvailable(CPM)
find_package(CPM)
# the install option has to be explicitly set to allow installation
CPMAddPackage(
GITHUB_REPOSITORY jarro2783/cxxopts
VERSION 2.2.1
OPTIONS "CXXOPTS_BUILD_EXAMPLES NO" "CXXOPTS_BUILD_TESTS NO" "CXXOPTS_ENABLE_INSTALL YES"
)this one create a warning but still working |
|
Of course, but that's the intended behaviour 😄 The warning I get there is: Which is ok because it is not a tagged version :) This one won't show the error even though it is not a release ;) |
I'm talking about this warning : CMake Warning at CMakeLists.txt:13 (find_package):
By not providing "FindCPM.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "CPM", but
CMake did not find one.
Could not find a package configuration file provided by "CPM" with any of
the following names:
CPMConfig.cmake
cpm-config.cmake
Add the installation prefix of "CPM" to CMAKE_PREFIX_PATH or set "CPM_DIR"
to a directory containing one of the above files. If "CPM" provides a
separate development package or SDK, be sure it has been installed.
I tested we need |
You have an error there. Without
|
|
@LecrisUT @TheLartians @CraigHutchinson you can try it with this example :
cmake_minimum_required(VERSION 3.14 FATAL_ERROR)
project(MyProject)
find_package(CPM.cmake QUIET)
if(NOT CPM.cmake_FOUND)
message(STATUS "Fetching CPM")
include(FetchContent)
FetchContent_Declare(
CPM.cmake
GIT_REPOSITORY https://github.com/flagarde/CPM.cmake.git
GIT_TAG FetchContent
)
FetchContent_MakeAvailable(CPM.cmake)
endif()
find_package(CPM.cmake REQUIRED)
CPMAddPackage("gh:fmtlib/fmt#7.1.3") |
|
Weird, I didn't get any notifications on this the whole week. Thanks for pinning me. Let me catch up to the conversation. |
|
@LecrisUT It happened to me from time to time, I don't know why. @TheLartians Concerning your qustion on the possibility to run a project offline when all has been previously loaded. I did a test with the example provided on my last comment. When all has been fetched, you can be offline and all is fine :) |
My vote is against this, it becomes very confusing for the user, and it is not guaranteed to be supported on cmake side in the long run, e.g. when they implement automatic namespacing. It is perfectly fine for the repo name and project name to have different case sensitivity and so on, since that is only present in
For this case you have |
| # For consumer using FetchContent | ||
| if(NOT ${CMAKE_PROJECT_NAME} STREQUAL ${PROJECT_NAME}) | ||
| # Check if the user download the repo with git and not in an official release | ||
| if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/.git" AND IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/.git") |
There was a problem hiding this comment.
I've recently found out about this approach:
.gitattributes and .git_archival.txt (this name is arbitrary).
Basically whenever Github creates the tar ball, it will change the content of .git_archival.txt with appropriate values. E.g.:
- Tagged version:
describe-name: v0.2.0 - Branch version:
describe-name: v0.2.0-9-gc4469f6
Let's go with this approach. We don't need to worry about the versions before this, because you can't use ExternalProject on those versions. Here's a list of the placeholders.
There was a problem hiding this comment.
To test locally, use:
$ git archive --format=tar HEAD --output=test.tar.gzThere was a problem hiding this comment.
@TheLartians As an alternative to hard-coded version numbering, would you agree to such approach, i.e.:
- Read
.git_archival.txtbeforeproject() - Reformat the version to be cmake compliant
- Use this variable as the version number
I have never tried this in a project, but could be very elegant if it can be implemented. Things to confirm:
- cmake accepts this format
-
CPM_MAJOR_VERSIONand so on are extractable - The commit is included in the version and can be retrieved
- It works on patch version branches, i.e. on
v0.38branch after pushing tagv0.38.1it continues to bev0.38.1-1-short_hash, while onmainit carries on withv0.38.0-1-short_hashand so on - Backup method for local development?
There was a problem hiding this comment.
Relevant PR here. For now this approach works because we don't actually build anything. Basically all of this is now compressed to
# Check if there is anything after the allowed format of `vX.Y.Z`, e.g. `vX.Y.Z-rc1`, `vX.Y.Z-9-gc449f6`, etc.
if (GIT_DESCRIBE MATCH "[v]?[0-9\\.]*.+")
set(CPM_RELEASE FALSE)
else ()
set(CPM_RELEASE TRUE)
endif ()
There was a problem hiding this comment.
This may, or may not be useful, i use this for versioning all my builds. Interested to add support for archival as this is new to knowledge to me but I parse the git-describe here. Note the use of EXPR was something I found useful to make it easy to handle all the variants. https://github.com/BareCpper/Version.cmake/blob/38de0dfb1686cfcc0578414587f06e6bcd7415c1/CMake/Version.cmake#L42
There was a problem hiding this comment.
Oh yeah, the regex is wrong because it is not maximally greedy.
That link is overkill though because the major.minor.patch.tweak format is already handled by the project(VERSION) section and the strip from ^([v]?([0-9\\.]+).*) over here, and we only care about if there are additional suffixes like -rc1 of any form. After some thought I think we can simplify it as ^[v]?[0-9\\.]*$ with inverted logic
| if(DEFINED CPM_VERSION AND CPM_RELEASE) | ||
| set(CURRENT_CPM_VERSION "${CPM_VERSION}${CPM_DEVELOPMENT}") | ||
| else() | ||
| set(CURRENT_CPM_VERSION 1.0.0-development-version) |
There was a problem hiding this comment.
With the .git_archival.txt we have more details on commit version. Should use that information.
Sure, it's possible to update an existing configuration, but you won't be able to configure from scratch. This is possible with # initial configure when online
> cmake -S . -B build1
[successful configure]
# we can now create arbitrary configurations when offline as CPM is cached
> cmake -S . -B build2
[successful configure]Using the # initial configure when online
> cmake -S . -B build1
[successful configure]
# any attempt to do a new configure when offline will result in an error
> cmake -S . -B build2
CMake Error at cpm.cmake-subbuild/cpm.cmake-populate-prefix/tmp/cpm.cmake-populate-gitclone.cmake:39 (message):
Failed to clone repository: 'https://github.com/flagarde/CPM.cmake.git'
make[2]: *** [cpm.cmake-populate-prefix/src/cpm.cmake-populate-stamp/cpm.cmake-populate-download] Error 1
make[1]: *** [CMakeFiles/cpm.cmake-populate.dir/all] Error 2
make: *** [all] Error 2
CMake Error at /opt/homebrew/Cellar/cmake/3.25.1/share/cmake/Modules/FetchContent.cmake:1616 (message):
Build step for cpm.cmake failed: 2
Call Stack (most recent call first):
/opt/homebrew/Cellar/cmake/3.25.1/share/cmake/Modules/FetchContent.cmake:1756:EVAL:2 (__FetchContent_directPopulate)
/opt/homebrew/Cellar/cmake/3.25.1/share/cmake/Modules/FetchContent.cmake:1756 (cmake_language)
/opt/homebrew/Cellar/cmake/3.25.1/share/cmake/Modules/FetchContent.cmake:1970 (FetchContent_Populate)
CMakeLists.txt:15 (FetchContent_MakeAvailable)
-- Configuring incomplete, errors occurred!For me this is actually a pretty common use-case, e.g. for multi platform builds, when debugging CMake scripts or when working in air gapped environments, which is why I prefer to avoid any regression here. |
|
If you point it to the same download location, i.e. |
You could use CPM.cmake_ROOT to give to the second condiguration the path to the cpm inside the first one. Note that you could install cpm by itself on some folder (it would be an equivalent of CPM_SOURCE_CACHE |
We could cache the CPM.cmake_DIR to allow find_package to find it |
I would not say it's a regression as this way to obtain CPM is new, if possible it should be nice to fix this differences but each metthod have pros and cons |
|
@TheLartians Would it be ok for you if I add the possibility to Fetched CPM to understand CPM_SOURCE_CACHE but I think you should say to user the CMake Alternative mentionned by @LecrisUT is preferred FETCHCONTENT_SOURCE_DIR_CPM,? |
|
Let me summarize the points we need to fix for this PR :
Do I forget something @LecrisUT @TheLartians @CraigHutchinson |
On my side, I would push against naming the project I think this is a risky approach and I would request more thorough discussion on this. @TheLartians @CraigHutchinson, can we discuss the points for and against this approach? The only one that I understood was related to |
I agree with you, but I have not infos on this but I would say it could be very risky too and this is just a convention name for find_package. CPM looks as fine as CPM.cmake to me but CPM are safer concerning the potential problems we can trigger |
On my side it's only the decision of using Other than that, for an initial PR, I think its fine in this state. |
|
For me the hardcoding of the version is still a red flag as it would complicate the release pipeline and at some point I'll likely forget to update the version in the CMakeLists and the version specified in the CMakeLists would be out-of-sync with the project. Is the version specifier really required and no way to automate it on releases? |
|
@TheLartians, @LecrisUT has found a nice way to avoid the hardcoded version. It can be done automatically by git using .git_archival.txt and gitattributes magics. Maybe we coiuld split the PR and try to push this mechanism first ? |
|
@flagarde Quick comment, remove the |
|
@LecrisUT You mean archives would not be reproductible anymore? |
|
With the file in the comment it will. Some more reading is at: pypa/setuptools_scm#1033 Basically |
That's super interesting, I've never heard of this before and even googling hardly shows any information besides the |
https://github.com/LecrisUT/CMakeExtraUtils/blob/main/cmake/DynamicVersion.cmake See also the repo containing it |
|
@TheLartians I have never seen this before but it seems nice feature |
Base on the conversation on #437. This PR is using the "standard" CMake feature with find_apckage etc.. But the CPM.cmake people need to change the version in command project before releasing.
Should be a better solution than #437
This is an example with the new features provided by this PR in action:
The other feature is that the PR allow CPM to be installed on user system (for example)
then user can use previous cmake code and let CMake find CPM in a la CMake way :
As you can see this PR makes CPM followinf CMake standards for package