Adding PATCHES keyword. #558

ScottBailey · 2024-04-23T19:32:15Z

This PR makes the following changes:

PATCHES multi-value keyword added to CPMAddPackage()
updated README for the new command
updated cmake-format for the new command
added a simple positive path integration test

Example usage:

CPMAddPackage(                                                                                                                                   
  NAME     src_lib                                                                                                                               
  URL      file:///home/bailey/research/CPM/src_lib.tar                                                                                          
  URL_HASH SHA256=0ce4bb954871b9d0250e5edfdd288dc44b295db60b2b3b832668eab52763e99b                                                               
  PATCHES                                                                                                                                        
    000-srclib-2.patch                                                                                                                           
    000-srclib-3.patch                                                                                                                           
)

Patches are applied in the order listed using the patch executable. In case of a Windows host, if patch.exe is not immediately found, a second search for patch.exe alongside git.exe is performed.

This code change is essentially syntactical sugar. We take the element(s) from PATCHES and put them together with &&s behind a PATCH_COMMAND that is added to CPM_ARGS_UNPARSED_ARGUMENTS.

This functionality is available manually, but it's quite ugly.

The above command written manually looks something like this:

find_program(PATCH_EXECUTABLE pexe ...)
CPMAddPackage(
...
  PATCH_COMMAND "${pexe}\\\;-p\\\;<\\\;${CMAKE_CURRENT_LIST_DIR}/000-srclib-2.patch\\\;&&\\\;${pexe}\\\;-p\\\;<\\\;${CMAKE_CURRENT_LIST_DIR}/000-srclib-3.patch"
...
)

Gerodote · 2024-04-23T22:41:14Z

@ScottBailey I tried this

For my own setup it works as expected: It applies patches

But with docker linux I got a bug : It doesn't apply patches.

Here's the branch with my attempt to test it. Try docker build . . After some time of compiling cmake, git and zip it shall configure cmake code and give error at generating. When it gives error at generating, it means it didn't apply patches

I don't know why in the docker image it fails and on my native machine it works correctly. We need further investigation.

Gerodote · 2024-04-23T22:54:16Z

Hm, it seems removing CPM_SOURCE_CACHE triggers the error

ScottBailey · 2024-04-23T22:57:42Z

I don't know why in the docker image it fails and on my native machine it works correctly. We need further investigation.

Cool, thanks! If I have time, I'll investigate tomorrow.

Truthfully, patching in CMake has a lot of problems. It's not going to work in all cases. Where it fails, PATCH_COMMAND wouldn't be any better.

Ultimately, it makes dependency management much more convenient and intuitive.

Gerodote · 2024-04-23T23:18:57Z

I tried some debugging when it happens ( when CPM_SOURCE_CACHE is not set)

It seems to not invoke your function because of an if statement

ScottBailey · 2024-04-24T01:45:36Z

It seems to not invoke your function because of an if statement

Good catch, I think I may have fixed it in 311cd3f.

Gerodote · 2024-04-24T11:27:21Z

It seems to not invoke your function because of an if statement

Good catch, I think I may have fixed it in 311cd3f.

Yep, it seems to work now. Good job!

ScottBailey · 2024-04-25T13:56:20Z

Will close #494

ScottBailey · 2024-04-25T14:42:54Z

The PATCHES command is currently a gaping hole in CMake's fetch_content functionality. This PR addresses that in the simplest way possible.

@Gerodote Please feel free to review and comment on the change.
@TheLartians @iboB Please review and comment on this change as time allows.

TheLartians

Hey, thanks a lot for adding this feature and a test case, it does look very useful! In general this looks very good, I just have a few questions about potential code simplifications.

TheLartians · 2024-05-26T20:40:27Z

cmake/CPM.cmake

+ set(CPM_ARGS_UNPARSED_ARGUMENTS
+ ${temp_list}
+ PARENT_SCOPE
+ )


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?

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.

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

TheLartians · 2024-05-26T20:54:06Z

cmake/CPM.cmake

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


Do we need the branch for different CMake versions or can we simply use the get_filename_component version to keep the code more simple?

I am suspicious get_filename_component will soon be deprecated and eventually removed. I think we can simplify the code to use the older functions for now, but may still eventually need to update it to this. It may take years for it's removal though, so if your preference is for simpler code I'm fine with that.

Upon further reflection, I believe CMake itself will eventually support PATCHES for FetchContent() and this is likely to happen BEFORE get_filename_component() is deprecated. I'll make the change and run some tests.

This is a functionality that I truly want to see in a release!

Addressed in 523c14a

TheLartians · 2024-05-26T20:59:42Z

cmake/CPM.cmake

+ # 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)


Can we not use something like get_filename_component(ABSOLUTE_PATH ${PATCH_FILE} PATCH_FILE) to be compatible with CMake < 3.20?

I feel there was a difference between the two, but I can test with get_filename_component sometime next week.

Addressed in 523c14a

…l cmake versions 3.14 and above.

Adding PATCHES keyword.

4395363

ScottBailey marked this pull request as draft April 23, 2024 19:32

ScottBailey mentioned this pull request Apr 23, 2024

Enhancement: add option PATCHES in which we can pass filepathes for patches to apply to the lib/package #494

Open

Formatting fix.

7793284

Move cpm_add_patches() outside if/else scopes.

311cd3f

cmake-format: add PATCHES to CPMAddPackage.

ceef053

ScottBailey marked this pull request as ready for review April 25, 2024 13:54

Integration tests for PATCHES command.

5ed5dfe

ScottBailey mentioned this pull request Apr 26, 2024

Add support for ExternalProject_Add() / libjpeg #550

Closed

TheLartians reviewed May 26, 2024

View reviewed changes

Use get_filename_component() in place of cmake_path() for use with al…

523c14a

…l cmake versions 3.14 and above.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding PATCHES keyword. #558

Adding PATCHES keyword. #558

ScottBailey commented Apr 23, 2024 •

edited

Gerodote commented Apr 23, 2024 •

edited

Gerodote commented Apr 23, 2024 •

edited

ScottBailey commented Apr 23, 2024

Gerodote commented Apr 23, 2024

ScottBailey commented Apr 24, 2024

Gerodote commented Apr 24, 2024

ScottBailey commented Apr 25, 2024

ScottBailey commented Apr 25, 2024

TheLartians left a comment

TheLartians May 26, 2024

ScottBailey May 29, 2024

ScottBailey Jun 3, 2024

TheLartians May 26, 2024

ScottBailey May 29, 2024

ScottBailey Jun 3, 2024

ScottBailey Jun 3, 2024

TheLartians May 26, 2024

ScottBailey May 29, 2024

ScottBailey Jun 3, 2024

Adding PATCHES keyword. #558

Are you sure you want to change the base?

Adding PATCHES keyword. #558

Conversation

ScottBailey commented Apr 23, 2024 • edited

Gerodote commented Apr 23, 2024 • edited

Gerodote commented Apr 23, 2024 • edited

ScottBailey commented Apr 23, 2024

Gerodote commented Apr 23, 2024

ScottBailey commented Apr 24, 2024

Gerodote commented Apr 24, 2024

ScottBailey commented Apr 25, 2024

ScottBailey commented Apr 25, 2024

TheLartians left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottBailey commented Apr 23, 2024 •

edited

Gerodote commented Apr 23, 2024 •

edited

Gerodote commented Apr 23, 2024 •

edited