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

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

Open
Gerodote opened this issue Aug 7, 2023 · 16 comments
Labels
enhancement New feature or request

Comments

@Gerodote
Copy link

Gerodote commented Aug 7, 2023

Assume we have a bad library that needs to be patched and we don't want to create a pull request, wait until a miracle happens and so on, or library just don't have github/gitlab pull request capability.

We would like to make a patch.

How to?

  1. Create patch:
    somehow get the lib
    cd the_lib_folder
    git init
    git add .
    git commit -m "initial"
  2. edit something, use your preferred method to change the code
  3. get your edits as a patch file(s) :
    git diff | tee /path_to_your_package/patches/pkg_name/patch_name.patch
  4. apply patches.
    apply -p1 -rnN -d ${library_name_SOURCE_DIR} -i filepath_to_the_patch
    there's also a git am command, you can find it somewhere at internet just googling, it will do the same, maybe its better option because IDK is the apply command portable on Windows and other environments.

How to apply patches in cmake ?
Current solution:

CPMAddPackage(pkg_name ...)
execute_process(COMMAND apply -rnN -p1 -d ${library_name_SOURCE_DIR} -i ${CMAKE_CURRENT_SOURCE_DIR}/patches/pkg_name/patch_name.patch  ... ) # -N is essential. it won't allow patch twice.

What is the proposal?

Add PATCHES option in CPMAddPackage . ( Maybe in CPMFindPackage too ).
It should take filepathes for patches. e.g. it should take ${CMAKE_CURRENT_SOURCE_DIR}/patches/${pkg_name}/* ( it can be a list of patches, which we could get using
file(GLOB_RECURSE pkg_name_patches CONFIGURE_DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/patches/pkg_name/*.patch )

@Gerodote
Copy link
Author

I've dug into this, and it seems git apply is so tangled with git base directory that it's almost impossible to use... stackoverflow link

Also, if you don't have apply command, it seems patch does exactly the same with the same flags.

@ScottBailey
Copy link
Contributor

CPM already has the patch command, is this to add more convenience?

If this is to add convenience, CPM could do find_package(Patch) for the patch command and simply apply it for each file or error out if the command is not available. I do not like to acquire libraries as git repos. I prefer to get archives.

This would be much better if cmake -E patch was a thing!

At any rate, one can currently do something like:

find_package(Patch REQUIRED)

# magic_enum
#  Reflection for enumerations, pretty cool really.
#  Note the limitations: https://github.com/Neargye/magic_enum/blob/master/doc/limitations.md
set(CMAKE_FOLDER "${BASE_FOLDER}/magic_enum")
CPMAddPackage(
  NAME magic_enum
  URL      https://github.com/Neargye/magic_enum/archive/refs/tags/v0.9.5.tar.gz
  URL_HASH SHA256=44ad80db5a72f5047e01d90e18315751d9ac90c0ab42cbea7a6f9ec66a4cd679
  SYSTEM   True
  PATCH_COMMAND
    "${Patch_EXECUTABLE}" -p1 < "${CMAKE_CURRENT_LIST_DIR}/magic_enum_format.patch"
)

@Gerodote
Copy link
Author

Gerodote commented Mar 18, 2024

@ScottBailey
I don't believe it does something ( it's only presence of thing in CPM )
image

Here's just search at codebase of cpm for word patch

@ScottBailey
Copy link
Contributor

PATCH_COMMAND is sent as part of CPM_ARGS_UNPARSED_ARGUMENTS to CMake propper without modification.

It would be GREAT if cmake -E patch existed. But it doesn't. Yet.

@Gerodote
Copy link
Author

Gerodote commented Mar 18, 2024

I've given such command to CPM....(PATCH_COMMAND):

/usr/bin/patch  -rnN -p1  <  /home/paxu/code/experiments/ModernCppStarterCPMBoostExample/patches/boost/boost_cmake_enable_install_rules_for_add_subdirectory_case.patch  /home/paxu/code/experiments/ModernCppStarterCPMBoostExample/patches/boost/boost_cmake_give_all_boost_deps_please.patch 

But it doesn't work

Example I tried:

set(TRY_BOOST_VERSION "1.84.0")
set(BOOST_NOT_HEADER_ONLY_COMPONENTS_THAT_YOU_NEED "thread")
set(BOOST_HEADER_ONLY_COMPONENTS_THAT_YOU_NEED "asio")
set(BOOST_INCLUDE_LIBRARIES
      "${BOOST_NOT_HEADER_ONLY_COMPONENTS_THAT_YOU_NEED};${BOOST_HEADER_ONLY_COMPONENTS_THAT_YOU_NEED}"
  )



  find_package(Patch REQUIRED)

  set(PATCH_COMMAND_ARGS " -rnN -p1 ")
  # set(PATCH_COMMAND_SOURCE " -d " )
  set(PATCH_COMMAND_INPUT_PATCHES " -i ")
  file(GLOB_RECURSE patches_for_boost CONFIGURE_DEPENDS
       "${CMAKE_CURRENT_SOURCE_DIR}/patches/boost/*.patch"
  )
  set(PATCH_COMMAND_FOR_CPM "${Patch_EXECUTABLE} ${PATCH_COMMAND_ARGS} < ")
  foreach(patch_filename IN LISTS patches_for_boost)
    string(APPEND PATCH_COMMAND_FOR_CPM " ${patch_filename} ")
  endforeach()
  message(DEBUG "Patch command: ${PATCH_COMMAND_FOR_CPM}")
  CPMAddPackage(
    NAME Boost
    URL "https://github.com/boostorg/boost/releases/download/boost-${TRY_BOOST_VERSION}/boost-${TRY_BOOST_VERSION}.tar.xz"
    OPTIONS "BOOST_ENABLE_CMAKE ON"
    PATCH_COMMAND ${PATCH_COMMAND_FOR_CPM}
  )
  set(IS_BOOST_LOCAL OFF)

a patch for example:

diff --git a/tools/cmake/include/BoostRoot.cmake b/tools/cmake/include/BoostRoot.cmake
index e93f9071..f0380b39 100644
--- a/tools/cmake/include/BoostRoot.cmake
+++ b/tools/cmake/include/BoostRoot.cmake
@@ -108,7 +108,7 @@ else()
   endif()
 
   set(BUILD_TESTING OFF)
-  set(BOOST_SKIP_INSTALL_RULES ON)
+  set(BOOST_SKIP_INSTALL_RULES OFF)
 
 endif()
 

I see the result in cmake logs, that it doesn't apply
for example:

...
|| -- Enabling installation for 'asio'
|| -- boost_install_target: not installing target 'boost_asio' due to BOOST_SKIP_INSTALL_RULES=ON
...

Instead of

|| -- Enabling installation for 'asio'

@ScottBailey
Copy link
Contributor

I think your syntax for patch is wrong. I appreciate that you want to simply apply all the patches, but why dont you take a step back and do this instead:

  
  set(PATCH_ARGS "")

  CPMAddPackage(
    NAME Boost
    URL "https://github.com/boostorg/boost/releases/download/boost-${TRY_BOOST_VERSION}/boost-${TRY_BOOST_VERSION}.tar.xz"
    OPTIONS "BOOST_ENABLE_CMAKE ON"
    PATCH_COMMAND
       "${Patch_EXECUTABLE}" ${PATCH_ARGS} -p1 < "${CMAKE_CURRENT_SOURCE_DIR}/patches/boost/000boost.patch"
       "${Patch_EXECUTABLE}" ${PATCH_ARGS} -p1 < "${CMAKE_CURRENT_SOURCE_DIR}/patches/boost/001boost.patch"
  )

I know this works. And maybe you/CPM have your quotes in the wrong place?

@ScottBailey
Copy link
Contributor

ScottBailey commented Mar 18, 2024

Here is the cmake issue for adding cmake -E patch.

This CPM issue could be done by adding a PATCH option that took a list of files and appended "${PatchEXECUTABLE}" -p1 < "${patch_file_path}" to CPM_ARGS_UNPARSED_ARGUMENTS.

This is a pretty quick change. Writing the documentation or the automated tests will take longer than the code itself.

CMake is not moving on that patch issue, so I'm hesitant to recommend we go forward on this. BUT maybe it is the right thing to do?

Patching is an essential part of package management, it might be nice if it was easier.

@TheLartians @iboB Opinions?

@Gerodote
Copy link
Author

Gerodote commented Mar 18, 2024

PATCH_COMMAND
   "${Patch_EXECUTABLE}" ${PATCH_ARGS} -p1 < "${CMAKE_CURRENT_SOURCE_DIR}/patches/boost/000boost.patch"

I know this works. And maybe you/CPM have your quotes in the wrong place?

It seems to not to.
Example I tried:

find_package(Patch REQUIRED)
set(PATCH_COMMAND_ARGS "")
  CPMAddPackage(
    NAME Boost
    URL "https://github.com/boostorg/boost/releases/download/boost-1.84.0/boost-1.84.0.tar.xz"
    OPTIONS "BOOST_ENABLE_CMAKE ON" 
    PATCH_COMMAND "${Patch_EXECUTABLE}" ${PATCH_COMMAND_ARGS} -p1 < "${CMAKE_CURRENT_SOURCE_DIR}/patches/boost/boost_cmake_enable_install_rules_for_add_subdirectory_case.patch"
  )

as you see, it's exactly like you proposed . And yes, I deleted cache of CPM, deleted build folder, and it's still the same

@ScottBailey
Copy link
Contributor

Got a simple repo? I know the magic_enum patch sample I have works. What OS are you on?

@Gerodote
Copy link
Author

Gentoo linux.

@Gerodote
Copy link
Author

Gerodote commented Mar 18, 2024

Like you can clone my repo for example and try it in docker: https://github.com/Gerodote/ModernCppStarterExampleBoostCmake

git clone https://github.com/Gerodote/ModernCppStarterExampleBoostCmake
cd ModernCppStarterExampleBoostCmake
docker build . ( yes, it's gonna compile git and almost latest cmake , then it's gonna compile the project)
You'll see the the problem.

@ScottBailey
Copy link
Contributor

ScottBailey commented Mar 18, 2024

Try this instead:

  CPMAddPackage(
    NAME Boost
    URL "https://github.com/boostorg/boost/releases/download/boost-${TRY_BOOST_VERSION}/boost-${TRY_BOOST_VERSION}.tar.xz"
    PATCH_COMMAND
      "${Patch_EXECUTABLE}" ${PATCH_COMMAND_ARGS} -p1 < "${CMAKE_CURRENT_SOURCE_DIR}/patches/boost/boost_cmake_enable_install_rules_for_add_subdirectory_case.patch"
      &&
      "${Patch_EXECUTABLE}" ${PATCH_COMMAND_ARGS} -p1 < "${CMAKE_CURRENT_SOURCE_DIR}/patches/boost/boost_cmake_give_all_boost_deps_please.patch"
    OPTIONS "BOOST_ENABLE_CMAKE ON"
  )

OPTIONS must be the final the argument.

And PATCH_COMMAND sucks for our usage. I've shown you a way to make this work; however, I think we could add a PATCH command that would be much better suck less.

@Gerodote
Copy link
Author

Gerodote commented Mar 18, 2024

Yep, it works

I tried some stuff.

  1. It seems PATCH_COMMAND only works if its value is a list of strings, not one string
  2. here's the working example with automation for looking patches and so on:
  find_package(Patch REQUIRED)
  set(PATCH_COMMAND_ARGS "-rnN")
  
  file(GLOB_RECURSE patches_for_boost CONFIGURE_DEPENDS
       "${CMAKE_CURRENT_SOURCE_DIR}/patches/boost/*.patch"
  )
  
  set(PATCH_COMMAND_FOR_CPM_BASE "${Patch_EXECUTABLE}" ${PATCH_COMMAND_ARGS}
    -p1 < )
    
  set(PATCH_COMMAND_FOR_CPM "")
  foreach(patch_filename IN LISTS patches_for_boost)
    list(APPEND PATCH_COMMAND_FOR_CPM ${PATCH_COMMAND_FOR_CPM_BASE})
    list(APPEND PATCH_COMMAND_FOR_CPM ${patch_filename})
    list(APPEND PATCH_COMMAND_FOR_CPM &&)
  endforeach()
  list(POP_BACK PATCH_COMMAND_FOR_CPM)
  
  message(DEBUG "Patch command: ${PATCH_COMMAND_FOR_CPM}")
  
  CPMAddPackage(
    NAME Boost
    URL "https://github.com/boostorg/boost/releases/download/boost-1.84.0/boost-1.84.0.tar.xz"
        PATCH_COMMAND ${PATCH_COMMAND_FOR_CPM}
    OPTIONS "BOOST_ENABLE_CMAKE ON"
  )

@Gerodote
Copy link
Author

Gerodote commented Mar 18, 2024

Thank you, Scott Bailey

@ScottBailey
Copy link
Contributor

My pleasure. Now lets see what everyone else thinks about adding PATCH or maybe PATCHES...

@ScottBailey
Copy link
Contributor

@Gerodote I wrote #558 to address adding patch files. Check it out and tell me what you think.

It needs some unit or integration tests before I move it from draft state.

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

No branches or pull requests

3 participants