Skip to content

Adding PATCHES keyword. #558

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

Merged
merged 7 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ Afterwards, any targets defined in the dependency can be used directly.
CPMAddPackage(
NAME # The unique name of the dependency (should be the exported target's name)
VERSION # The minimum version of the dependency (optional, defaults to 0)
PATCHES # Patch files to be applied sequentially using patch and PATCH_OPTIONS (optional)
OPTIONS # Configuration options passed to the dependency (optional)
DOWNLOAD_ONLY # If set, the project is downloaded, but not configured (optional)
[...] # Origin parameters forwarded to FetchContent_Declare, see below
Expand Down
1 change: 1 addition & 0 deletions cmake/.cmake-format-additional_commands-cpm
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ parse:
EXCLUDE_FROM_ALL: 1
SYSTEM: 1
SOURCE_SUBDIR: 1
PATCHES: +
OPTIONS: +
cpmfindpackage:
pargs:
Expand Down
79 changes: 78 additions & 1 deletion cmake/CPM.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,80 @@ function(cpm_check_git_working_dir_is_clean repoPath gitTag isClean)

endfunction()

# method to add PATCH_COMMAND to CPM_ARGS_UNPARSED_ARGUMENTS for PATCHES files.
function(cpm_add_patches)
# Return if no patch files are supplied.
if(NOT ARGN)
return()
endif()

# Provide a small warning.
if(CMAKE_VERSION VERSION_LESS "3.20")
message(
AUTHOR_WARNING
"Versions of CMake less than 3.20 may need to supply patch files as absolute paths."
)
endif()

# Find the patch program.
find_program(PATCH_EXECUTABLE patch)
if(WIN32 AND NOT PATCH_EXECUTABLE)
# The Windows git executable is distributed with patch.exe. Find the path to the executable, if
# it exists, then search `../../usr/bin` for patch.exe.
find_package(Git QUIET)
if(GIT_EXECUTABLE)
if(CMAKE_VERSION VERSION_GREATER_EQUAL "3.20")
cmake_path(GET GIT_EXECUTABLE PARENT_PATH extra_search_path)
cmake_path(GET extra_search_path PARENT_PATH extra_search_path)
else()
get_filename_component(extra_search_path ${GIT_EXECUTABLE} DIRECTORY)
get_filename_component(extra_search_path ${extra_search_path} DIRECTORY)
get_filename_component(extra_search_path ${extra_search_path} DIRECTORY)
endif()
find_program(PATCH_EXECUTABLE patch HINTS "${extra_search_path}/usr/bin")
endif()
endif()
if(NOT PATCH_EXECUTABLE)
message(FATAL_ERROR "Couldn't find `patch` executable to use with PATCHES keyword.")
endif()

# Create a temporary
set(temp_list ${CPM_ARGS_UNPARSED_ARGUMENTS})

# Ensure each file exists (or error out) and add it to the list.
set(first_item True)
foreach(PATCH_FILE ${ARGN})
if(NOT EXISTS "${PATCH_FILE}")
if(NOT EXISTS "${CMAKE_CURRENT_LIST_DIR}/${PATCH_FILE}")
message(FATAL_ERROR "Couldn't find patch file: '${PATCH_FILE}'")
endif()
set(PATCH_FILE "${CMAKE_CURRENT_LIST_DIR}/${PATCH_FILE}")
endif()
# Convert to absolute path for CMake 3.20 (available since March 3rd, 2021) and later, this may
# cause users with older versions of CMake to strictly call out an absolute path.
if(CMAKE_VERSION VERSION_GREATER_EQUAL "3.20")
cmake_path(ABSOLUTE_PATH PATCH_FILE)
endif()
# The first patch entry must be preceded by "PATCH_COMMAND" while the following items are
# preceded by "&&".
if(first_item)
set(first_item False)
list(APPEND temp_list "PATCH_COMMAND")
else()
list(APPEND temp_list "&&")
endif()
# Add the patch command to the list
list(APPEND temp_list "${PATCH_EXECUTABLE}" "-p1" "<" "${PATCH_FILE}")
endforeach()

# Move temp out into parent scope.
set(CPM_ARGS_UNPARSED_ARGUMENTS
${temp_list}
PARENT_SCOPE
)
Comment on lines +523 to +526
Copy link
Member

Choose a reason for hiding this comment

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

Generally, if modifying the parent scope, I feel it would be a good idea to provide the variable name in the parent as an argument to the function to make it easier for us to read the called function. Do you think it would make sense here as well?

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'm not sure I understand the suggestion? Can you point me to an example?

In this case, we put cpm_add_patches into a function to keep the code more readable. I suppose we could send CPM_ARGS_UNPARSED_ARGUMENTS in as an argument, but I feel like the documentation would always be VARIABLE_TO_UPDATE _must_ _be_ CPM_ARGS_UNPARSED_ARGUMENTS. This could be more confusing overall, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheLartians 523c14a addresses the other comments, but I'm not sure what to do here... Suggestions? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I see your point that the function is basically just an outsourced section of a larger function, but in general I like to have clearly defined input and outputs in functions - in CMake's case using arguments so the caller can decide what goes in and comes out. In my experience ist helps a lot with readability. In any way it's just a style preference and not a blocker for this PR.


endfunction()

# method to overwrite internal FetchContent properties, to allow using CPM.cmake to overload
# FetchContent calls. As these are internal cmake properties, this method should be used carefully
# and may need modification in future CMake versions. Source:
Expand Down Expand Up @@ -537,7 +611,7 @@ function(CPMAddPackage)
CUSTOM_CACHE_KEY
)

set(multiValueArgs URL OPTIONS DOWNLOAD_COMMAND)
set(multiValueArgs URL OPTIONS DOWNLOAD_COMMAND PATCHES)

cmake_parse_arguments(CPM_ARGS "" "${oneValueArgs}" "${multiValueArgs}" "${ARGN}")

Expand Down Expand Up @@ -628,6 +702,7 @@ function(CPMAddPackage)
SOURCE_DIR "${PACKAGE_SOURCE}"
EXCLUDE_FROM_ALL "${CPM_ARGS_EXCLUDE_FROM_ALL}"
SYSTEM "${CPM_ARGS_SYSTEM}"
PATCHES "${CPM_ARGS_PATCHES}"
OPTIONS "${CPM_ARGS_OPTIONS}"
SOURCE_SUBDIR "${CPM_ARGS_SOURCE_SUBDIR}"
DOWNLOAD_ONLY "${DOWNLOAD_ONLY}"
Expand Down Expand Up @@ -683,6 +758,8 @@ function(CPMAddPackage)
set(CPM_FETCHCONTENT_BASE_DIR ${CMAKE_BINARY_DIR}/_deps)
endif()

cpm_add_patches(${CPM_ARGS_PATCHES})

if(DEFINED CPM_ARGS_DOWNLOAD_COMMAND)
list(APPEND CPM_ARGS_UNPARSED_ARGUMENTS DOWNLOAD_COMMAND ${CPM_ARGS_DOWNLOAD_COMMAND})
elseif(DEFINED CPM_ARGS_SOURCE_DIR)
Expand Down
13 changes: 13 additions & 0 deletions test/integration/templates/using-patch-adder/lists.in.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
cmake_minimum_required(VERSION 3.14 FATAL_ERROR)

project(using-patch-adder)

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)

include("%{cpm_path}")

%{packages}

add_executable(using-patch-adder using-patch-adder.cpp)

target_link_libraries(using-patch-adder adder)
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
diff --git a/code/adder/adder.hpp b/code/adder/adder.hpp
index fdb9324..7c2fa00 100644
--- a/code/adder/adder.hpp
+++ b/code/adder/adder.hpp
@@ -1,6 +1,6 @@
#pragma once

-namespace adder
+namespace patched
{
int add(int a, int b);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
diff --git a/code/adder/adder.cpp b/code/adder/adder.cpp
index fc6e767..44b1197 100644
--- a/code/adder/adder.cpp
+++ b/code/adder/adder.cpp
@@ -1,6 +1,6 @@
#include "adder.hpp"

-namespace adder
+namespace patched
{
int add(int a, int b)
{
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#include <adder/adder.hpp>
#include <cstdio>

int main() {
int sum = patched::add(5, 3);
std::printf("%d\n", sum);
return 0;
}
27 changes: 27 additions & 0 deletions test/integration/test_patches_command.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
require_relative './lib'

# Tests using a multi-argumenet PATCHES command to fetch and modify a dependency

class DownloadCommand < IntegrationTest

def test_fetch_dependency_using_download_command
prj = make_project from_template: 'using-patch-adder'

prj.create_lists_from_default_template package: <<~PACK
set(DOWNLOAD_DIR ${CMAKE_BINARY_DIR}/_deps/testpack-adder-src)
CPMAddPackage(
NAME testpack-adder
DOWNLOAD_COMMAND git clone --depth 1 --branch v1.0.0 https://github.com/cpm-cmake/testpack-adder.git ${DOWNLOAD_DIR}
OPTIONS "ADDER_BUILD_TESTS OFF" "ADDER_BUILD_EXAMPLES OFF"
PATCHES
patches/001-test_patches_command.patch
patches/002-test_patches_command.patch
)
PACK

# configure with unpopulated cache
assert_success prj.configure
assert_success prj.build
end

end
Loading