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
added example/highway to show real world usage

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"
...
)

Arniiiii · 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.

Arniiiii · 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.

Arniiiii · 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.

Arniiiii · 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!

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.

cmake/CPM.cmake

…l cmake versions 3.14 and above.

TheLartians

Thanks again for the feature and the changes!

Looks great and I'm happy that we no longer run different code based on the current CMake version! I have two small comments, both of which are not blocking merging the PR.

I'll keep the PR open a bit in case you want to make changes and release the feature in the next days.

TheLartians · 2024-06-07T16:51:26Z

cmake/CPM.cmake

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


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.

README.md

TheLartians

This looks great, thanks again for taking the time to implement this feature and the changes requested!

TheLartians · 2024-06-12T13:46:00Z

Feature is released in v0.40.0! 🎉

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.

TheLartians previously approved these changes Jun 7, 2024

View reviewed changes

TheLartians mentioned this pull request Jun 7, 2024

when use cache the depends lib appeared outside the project in clion #563

Open

Added an example and improved comment for cpm_add_patches.

9347302

ScottBailey dismissed TheLartians’s stale review via 9347302 June 9, 2024 02:20

TheLartians approved these changes Jun 12, 2024

View reviewed changes

TheLartians merged commit d416d9b into cpm-cmake:master Jun 12, 2024
8 checks passed

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

Loading

Arniiiii commented Apr 23, 2024 •

edited

Loading

Arniiiii commented Apr 23, 2024 •

edited

Loading

ScottBailey commented Apr 23, 2024

Arniiiii commented Apr 23, 2024

ScottBailey commented Apr 24, 2024

Arniiiii 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 Jun 7, 2024

TheLartians left a comment

TheLartians Jun 7, 2024

TheLartians left a comment

TheLartians commented Jun 12, 2024

Adding PATCHES keyword. #558

Adding PATCHES keyword. #558

Conversation

ScottBailey commented Apr 23, 2024 • edited Loading

Arniiiii commented Apr 23, 2024 • edited Loading

Arniiiii commented Apr 23, 2024 • edited Loading

ScottBailey commented Apr 23, 2024

Arniiiii commented Apr 23, 2024

ScottBailey commented Apr 24, 2024

Arniiiii 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

TheLartians May 26, 2024

Choose a reason for hiding this comment

ScottBailey May 29, 2024

Choose a reason for hiding this comment

ScottBailey Jun 3, 2024

Choose a reason for hiding this comment

TheLartians Jun 7, 2024

Choose a reason for hiding this comment

TheLartians left a comment

Choose a reason for hiding this comment

TheLartians Jun 7, 2024

Choose a reason for hiding this comment

TheLartians left a comment

Choose a reason for hiding this comment

TheLartians commented Jun 12, 2024

ScottBailey commented Apr 23, 2024 •

edited

Loading

Arniiiii commented Apr 23, 2024 •

edited

Loading

Arniiiii commented Apr 23, 2024 •

edited

Loading