Allow to use FetchContent to get CPM #437
Conversation
|
Maybe worth making an |
|
You do not use FetchContent? |
|
@ClausKlein this PR allows consumer to use FetchContent to download CPM (not using To my understanding FetchContent automatically retries and can check the download has been performed correctly etc... |
|
@flagarde please use |
|
About the |
|
@LecrisUT https://pre-commit.ci/ can automatically send PR with formatted code |
|
@ClausKlein done |
|
About the About the pre-commit, these can also add commits directly in the PR. |
|
@LecrisUT find_package could be nice for the install maybe the consumer just want to use it and not install it but it should be possible to have option to include to ALL or not |
CMakeLists.txt
Outdated
| if(GIT_EXECUTABLE) | ||
| # 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 CURRENT_CPM_VERSION | ||
| RESULT_VARIABLE GIT_DESCRIBE_ERROR_CODE | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| ) | ||
| if(NOT GIT_DESCRIBE_ERROR_CODE AND NOT CURRENT_CPM_VERSION STREQUAL "") | ||
| string(SUBSTRING ${CURRENT_CPM_VERSION} 1 -1 CURRENT_CPM_VERSION) | ||
| else() | ||
| set(CURRENT_CPM_VERSION 1.0.0-development-version) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Why use git again if you will use FetchContent?
May you show us a usage example?
There was a problem hiding this comment.
If I understand this correctly, it is to get the CPM version from the git tags currently used. I would rather prefer we make the version on the cmake be the main version source. This would require the maintainers to agree to such change of workflow.
It might be possible to link/create a github action to read the cmake version and push tags automatically 🤔
There was a problem hiding this comment.
Yes it is to extract the version from the tag we are asking. I agree it would be better to include the right version to the project but the drawback is that the user could ask to use the main branch without any tags and would receive the version that is refering to the latest tags/version but still be upstream if the have pulled changes before releasing a new version... Using git to acces the tag, if the consumer ask to point to a commit or a branch (main for example), than he will receive the warning that he is using a unversionned cpm (because HEAD is not tagged so CURRENT_CPM_VERSION will be set to '1.0.0-development-version' which will trigger a warning on CPM.cmake). It's the simplest way I have found to do this, and as we are using Fetchcontent we are sure Git is present.
There was a problem hiding this comment.
I can't quite follow this. The user can get a specific version from the tags, and for debugging/testing they'll get the main branch. What issues are we encountering in the latter? Is it that versions are coded in the cmake file? If that's the case, more appropriately it would be to switch the workflow to a cmake project versioning and/or add release branches to remove the warnings from which we specify the release tags.
There was a problem hiding this comment.
Imagine your master branch have some commit not part of a tag, and not yet ready for an updated version you will be between for example the latest version let's say 0.39.0 and the next wonderful version 0.40.0. Which version to put ? 0.39.0 is impossible because you are not anymore in 0.39.0 but not yet on 0.40.0
There was a problem hiding this comment.
it could be possible to have an empty version for not released commit I agree but this required more change on the workflow
There was a problem hiding this comment.
The general approach is to append the short hash or branch to the last known version (as PROJECT_VERSION_TWEAK) which would be derived from the hardcoded value in CMakeLists.txt.
But, since this would not be intended for production I think it is ok to have some ambiguity in the version. It would be possible to get the latest version everytime as well by setting it to github latest release tar ball.
If we still want to get the version being requested without git, I would investigate if FetchContent exposes any variable related to how it is being called and take the value from there.
| @@ -0,0 +1,23 @@ | |||
| cmake_minimum_required(VERSION 3.14 FATAL_ERROR) | |||
|
|
|||
| find_package(Git REQUIRED QUIET) | |||
There was a problem hiding this comment.
remove REQUIRED please!
There was a problem hiding this comment.
Could you explain why ? I agree that FetchContent needs it so it should be safe but it should not hurt just for safety / be clear to the user that it is needed
There was a problem hiding this comment.
Maybe you can FetchContent from tarball release on systems without git (maybe a lightweight docker)
There was a problem hiding this comment.
from CMake doc:
The QUIET option disables informational messages, including those indicating that the package cannot be found if it is not REQUIRED.
The REQUIRED option stops processing with an error message if the package cannot be found.
There was a problem hiding this comment.
That is the needed behaviour, Git is needed to extract the version
There was a problem hiding this comment.
Maybe you can FetchContent from tarball release on systems without git (maybe a lightweight docker)
I agree but without Git the purpose of CPM is very limited. In this case people could still use get_cpm. My idea was to provide an alternative to get_cpm. If we want to get ou of Git this requires more changes (Maybe in next PR :) )
There was a problem hiding this comment.
I think it is still useful as is because the user does not have to include an external file of get_cpm and instead use more native commands cmake commands.
|
|
||
| find_package(Git REQUIRED QUIET) | ||
|
|
||
| project(CPM LANGUAGES NONE) |
There was a problem hiding this comment.
IMHO: The project() should be before find_package()
There was a problem hiding this comment.
Yes should be safer indeed
|
|
||
| project(CPM LANGUAGES NONE) | ||
|
|
||
| if(GIT_EXECUTABLE) |
There was a problem hiding this comment.
NOTE: this is not needed and will not reached with REQUIRED set
|
Sorry to bother you, but this does not work in each use cases of FetchContent_Declare(_cpm URL https://github.com/cpm-cmake/CPM.cmake/archive/refs/tags/v0.37.0.zip)
FetchContent_MakeAvailable(_CPM)
add_subdirectory(${_cpm_SOURCE_DIR} CPM)I have applied your patch to this source dir: bash-3.2$ pwd
/Users/clausklein/Workspace/cpp/ModernCmakeStarter/build/user/_deps/_cpm-src
bash-3.2$ cat CMakeLists.txt
cmake_minimum_required(VERSION 3.14 FATAL_ERROR)
find_package(Git REQUIRED QUIET)
project(CPM LANGUAGES NONE)
if(GIT_EXECUTABLE)
# 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 CURRENT_CPM_VERSION
RESULT_VARIABLE GIT_DESCRIBE_ERROR_CODE
OUTPUT_STRIP_TRAILING_WHITESPACE
)
if(NOT GIT_DESCRIBE_ERROR_CODE AND NOT CURRENT_CPM_VERSION STREQUAL "")
string(SUBSTRING ${CURRENT_CPM_VERSION} 1 -1 CURRENT_CPM_VERSION)
else()
set(CURRENT_CPM_VERSION 1.0.0-development-version)
endif()
endif()
include("${CMAKE_CURRENT_SOURCE_DIR}/cmake/CPM.cmake")
bash-3.2$ git tag --points-at HEAD
v0.1.0
bash-3.2$ git remote -v
origin git@github.com:ClausKlein/ModernCmakeStarter.git (fetch)
origin git@github.com:ClausKlein/ModernCmakeStarter.git (push)
bash-3.2$ NOTE: this is the tag of my repo! |
|
I see. I guess it is because there is no .git on the downloaded repo' so the git program try to find one and goes up in the folder hierarchy and then found the .git in your repo that I imagine is v0.1.0. I think I can fix this checking if there is a .git on the source directory. If not i could parse the url and extract the version If it 8s possible to access to the URL variable you put on the command What do you think of this fix? I was thinking for git repo people would us the GIT_TAG command not the URL. But yes I should consider this option too |
|
I think it will be safer to provide the |
|
@LecrisUT I agree |
|
Ak no sorry, that will not help with For |
|
@LecrisUT @ClausKlein Based on our conversations I created a better solution (I think ...). Please have a look on #446. It should work with GIT_TAG and URL and have and install option for people who want to install it on there system and use 'find_package(CPM)' |
@flagarde Is this PR good to close now considering the new approach exists? |
it would be easier to recover if download fails etc..