-
Notifications
You must be signed in to change notification settings - Fork 201
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
Changes from 5 commits
4395363
7793284
311cd3f
ceef053
5ed5dfe
523c14a
9347302
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ parse: | |
EXCLUDE_FROM_ALL: 1 | ||
SYSTEM: 1 | ||
SOURCE_SUBDIR: 1 | ||
PATCHES: + | ||
OPTIONS: + | ||
cpmfindpackage: | ||
pargs: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
TheLartians marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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) | ||
TheLartians marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
@@ -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}") | ||
|
||
|
@@ -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}" | ||
|
@@ -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) | ||
|
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; | ||
} |
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 |
Uh oh!
There was an error while loading. Please reload this page.