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.

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

Adding PATCHES keyword. #558

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

Conversation

ScottBailey
Copy link
Contributor

@ScottBailey ScottBailey commented Apr 23, 2024

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
Copy link

Gerodote commented Apr 23, 2024

@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
Copy link

Gerodote commented Apr 23, 2024

Hm, it seems removing CPM_SOURCE_CACHE triggers the error

@ScottBailey
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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

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

@Gerodote
Copy link

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 ScottBailey marked this pull request as ready for review April 25, 2024 13:54
@ScottBailey
Copy link
Contributor Author

Will close #494

@ScottBailey
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants