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.

Sign up for GitHub

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

Fetch content #446

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

Fetch content #446

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
fb703d8
Add install and FetchContent
flagarde Feb 16, 2023
6b7ae0d
cmake-format
flagarde Feb 16, 2023
eb8461d
Fix
flagarde Feb 16, 2023
0711ac1
cmake-format
flagarde Feb 16, 2023
a70c613
Update CMakeLists.txt
flagarde Feb 16, 2023
8ae0a09
Update cmake/CPM.cmake
flagarde Feb 16, 2023
3e7e6d6
Update CMakeLists.txt
flagarde Feb 16, 2023
d908cfe
cmake-format
flagarde Feb 16, 2023
d2975d0
Update CMakeLists.txt
flagarde Feb 16, 2023
14bb75f
Update CMakeLists.txt
flagarde Feb 16, 2023
a0663be
Update CMakeLists.txt
flagarde Feb 16, 2023
fa5058c
Update CPM.cmake
flagarde Feb 16, 2023
03027e0
Update CMakeLists.txt
flagarde Feb 16, 2023
deeefc3
Updates based on comments
flagarde Mar 2, 2023
f02381c
Updates based on comments
flagarde Mar 2, 2023
fbf5763
Updates based on comments
flagarde Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
cmake_minimum_required(VERSION 3.14 FATAL_ERROR)

project(
CPM
VERSION 0.37.0
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of hardcoding the version in Git as this will require maintainers to be careful when creating new releases that the version and according tag are identical. Additionally the check below will force consumers to use make proper git clones including tags to ensure they are not in a development version.

How about instead using a mechanism like in the publish workflow to publish a downloadable zip archive of the relevant files with the current version on new tags?

Copy link

Choose a reason for hiding this comment

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

I would disagree on this. It makes the intent of the versioning more clear. For example, when navigating the git commits, it is easier to know which version the current commit is intended for from the CMakelists.txt file. The usual workflow for a release is:

  1. Push a versioned tag with the version marked in the CMakeLists.txt
  2. On next commit, increment version

Number 2. can be automated in publish action to always occur after a release is published. For the case of bug fix increments, those should be branched from the relevant version and merged back into the development branch. For the major version it should be done manually to indicate intent of api change.

LANGUAGES NONE
)

include(CMakePackageConfigHelpers)
configure_package_config_file(
"${CMAKE_CURRENT_SOURCE_DIR}/cmake/CPMConfig.cmake.in"
"${CMAKE_CURRENT_BINARY_DIR}/CPMConfig.cmake"
INSTALL_DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/CPM"
Copy link
Member

Choose a reason for hiding this comment

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

What if we would want to install multiple CPM versions side-by-side, would the previous version be overwritten?
I think the install destination should contain a version specifier as well.

Copy link

Choose a reason for hiding this comment

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

That is common practice. The installation via CPMConfig.cmake should in principle be managed by the package manager, in which case you have only 1 version. If you want to install locally multiple versions, you change CMAKE_INSTALL_PREFIX to an appropriate location with the correct version.

Including the verison in the CPMConfig.cmake filename is unsupported on the cmake side afaik.

)
write_basic_package_version_file(
"${CMAKE_CURRENT_BINARY_DIR}/CPMConfigVersion.cmake" COMPATIBILITY SameMajorVersion
ARCH_INDEPENDENT
Copy link
Member

Choose a reason for hiding this comment

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

Currently we update the minor version on new features, so compatibility should reflect it as well.

Suggested change
"${CMAKE_CURRENT_BINARY_DIR}/CPMConfigVersion.cmake" COMPATIBILITY SameMajorVersion
ARCH_INDEPENDENT
"${CMAKE_CURRENT_BINARY_DIR}/CPMConfigVersion.cmake" COMPATIBILITY SameMinorVersion
ARCH_INDEPENDENT

Copy link

Choose a reason for hiding this comment

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

Please add a note to remember to change this when there will be CPM v1.0.0. Also wouldn't it make sense to release v1.0.0 with this feature complete and tested? Any other items to be resolved before that can be released?

)

# For consumer using FetchContent
if(NOT ${CMAKE_PROJECT_NAME} STREQUAL ${PROJECT_NAME})
# Trick to use the find_package
configure_file(
"${CMAKE_CURRENT_SOURCE_DIR}/cmake/CPM.cmake" "${CMAKE_CURRENT_BINARY_DIR}/CPM.cmake" COPYONLY
)
set(CPM_DIR "${CMAKE_CURRENT_BINARY_DIR}")
# 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")
Copy link

@LecrisUT LecrisUT Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

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.

Copy link

Choose a reason for hiding this comment

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

To test locally, use:

$ git archive --format=tar HEAD --output=test.tar.gz

Copy link

@LecrisUT LecrisUT Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@TheLartians As an alternative to hard-coded version numbering, would you agree to such approach, i.e.:

  • Read .git_archival.txt before project()
  • 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_VERSION and 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.38 branch after pushing tag v0.38.1 it continues to be v0.38.1-1-short_hash, while on main it carries on with v0.38.0-1-short_hash and so on
  • Backup method for local development?

Copy link

Choose a reason for hiding this comment

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

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 ()

Copy link
Contributor

@CraigHutchinson CraigHutchinson Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

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()
endif()
flagarde marked this conversation as resolved.
Show resolved Hide resolved
find_package(CPM CONFIG REQUIRED NO_POLICY_SCOPE)
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@flagarde flagarde Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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 main to X.Y.Z branch
  • Rebase a commit that edits set(CPM_RELEASE OFF) -> set(CPM_RELEASE ON)
  • Tag release to this branch

Copy link
Contributor Author

@flagarde flagarde Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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()

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

We are over-complicating it here. This works just fine

Suggested change
find_package(CPM CONFIG REQUIRED NO_POLICY_SCOPE)
include(cmake/CPM.cmake)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it seems to work :)

Please have a try too.. just in case

Copy link

Choose a reason for hiding this comment

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

All good now. For an initial integration I'm 👍 on this.

endif()

if(NOT CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
flagarde marked this conversation as resolved.
Show resolved Hide resolved
# Without it : Unable to determine default CMAKE_INSTALL_LIBDIR directory because no target
# architecture is known. Please enable at least one language before including GNUInstallDirs.
enable_language(C)
include(GNUInstallDirs)
install(
FILES "${CMAKE_CURRENT_BINARY_DIR}/CPMConfig.cmake"
"${CMAKE_CURRENT_BINARY_DIR}/CPMConfigVersion.cmake"
"${CMAKE_CURRENT_SOURCE_DIR}/cmake/CPM.cmake"
DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/CPM"
)
endif()
6 changes: 5 additions & 1 deletion cmake/CPM.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ if(NOT COMMAND cpm_message)
endfunction()
endif()

set(CURRENT_CPM_VERSION 1.0.0-development-version)
if(DEFINED CPM_VERSION)
set(CURRENT_CPM_VERSION "${CPM_VERSION}${CPM_DEVELOPMENT}")
else()
set(CURRENT_CPM_VERSION 1.0.0-development-version)
Copy link

Choose a reason for hiding this comment

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

With the .git_archival.txt we have more details on commit version. Should use that information.

endif()

get_filename_component(CPM_CURRENT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}" REALPATH)
if(CPM_DIRECTORY)
Expand Down
5 changes: 5 additions & 0 deletions cmake/CPMConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
@PACKAGE_INIT@

include("${CMAKE_CURRENT_LIST_DIR}/CPM.cmake" NO_POLICY_SCOPE)

check_required_components(CPM)