Use the same patching script and command for all repos #584
Conversation
To simplify patching, the script used for the LLVM repo can be expanded for use on all repos. This also updates the newlib patch to use the same format as llvm-project and picolibc; using git's patch format means the .patch file is compatible with both git am and git apply, whereas a simple diff only works with git apply. This also adds an option to control the patch method used by the script, since using git am may be preferable to the default git apply. The script already supports the option, so all that is needed to pass down the selection from the CMake cache if present. Running git --check together with --apply --3way does not generatean error return code when the patches are valid but contain conflicts, so this particular combination is now marked as incompatible with the --restore_on_fail option, since this relies on --check generating an error.
cmake/fetch_newlib.cmake
Outdated
| @@ -11,14 +11,21 @@ if(NOT VERSIONS_JSON) | |||
| endif() | |||
| read_repo_version(newlib newlib) | |||
|
|
|||
| set(newlib_patch ${CMAKE_CURRENT_LIST_DIR}/../patches/newlib.patch) | |||
| set(patch_script ${CMAKE_CURRENT_LIST_DIR}/patch_repo.py) | |||
There was a problem hiding this comment.
There's some code duplication here. Perhaps it could be refactored into (yet) another include file that would define patch_dir, and a combined patch_script_command list variable containing all of ${Python3_EXECUTABLE} ${patch_script} ${patch_script_args}?
Then the LLVM file would still be able to have its special case of conditionally adding a second invocation of the whole thing, but there'd be just one place to add further refinements like GIT_PATCH_METHOD (which I like!)
There was a problem hiding this comment.
That would be neater, so I've wrapped it into a simple function which generates the required command for a given target folder, which the LLVM version can simply call twice.
There was a problem hiding this comment.
I think you've slightly improved on my suggestion, by folding all use of patch_dir into the include file instead of having it just defined there and used by the caller. I like it!
| @@ -36,10 +36,20 @@ def main(): | |||
| action="store_true", | |||
| help="If a patch in a series cannot be applied, restore the original state instead of leaving patches missing. Return code will be 2 instead of 1.", | |||
| ) | |||
| parser.add_argument( | |||
| "--3way", | |||
| action="store_true", | |||
There was a problem hiding this comment.
It looks as if --3way was previously unconditionally added, and now won't be added by default, because this new boolean argument defaults to off? Is that on purpose? I don't see anything in the commit message mentioning that it was.
There was a problem hiding this comment.
The --3way option has some side effects that I'm not sure were considered when it was added as the default; namely that it can seemingly apply a patch, but leave conflict markers in place. That will then cause the compile to fail, instead of flagging that there's an issue in patching. The expectation is for a user to deal with the conflicts, so it's less useful in an automated context. Using --3way shouldn't be necessary from what I've tried (and indeed the newlib patch command never used it), so I removed it from the default but left the option to enable it in the script.
I'll update the commit message when squashing to mention all this.
There was a problem hiding this comment.
Fine – that makes sense. If it's disabled on purpose and you say why, I'm happy.
| @@ -36,10 +36,20 @@ def main(): | |||
| action="store_true", | |||
| help="If a patch in a series cannot be applied, restore the original state instead of leaving patches missing. Return code will be 2 instead of 1.", | |||
| ) | |||
| parser.add_argument( | |||
| "--3way", | |||
| action="store_true", | |||
There was a problem hiding this comment.
Fine – that makes sense. If it's disabled on purpose and you say why, I'm happy.
cmake/fetch_newlib.cmake
Outdated
| @@ -11,14 +11,21 @@ if(NOT VERSIONS_JSON) | |||
| endif() | |||
| read_repo_version(newlib newlib) | |||
|
|
|||
| set(newlib_patch ${CMAKE_CURRENT_LIST_DIR}/../patches/newlib.patch) | |||
| set(patch_script ${CMAKE_CURRENT_LIST_DIR}/patch_repo.py) | |||
There was a problem hiding this comment.
I think you've slightly improved on my suggestion, by folding all use of patch_dir into the include file instead of having it just defined there and used by the caller. I like it!
cmake/patch_repo.cmake
Outdated
| # patch_repo.py script using a target set of patches. | ||
|
|
||
| function(get_patch_command patch_dir patch_command_out) | ||
| set(patch_script ${CMAKE_CURRENT_LIST_DIR}/patch_repo.py) |
There was a problem hiding this comment.
I think that using ${CMAKE_CURRENT_LIST_DIR} in this context means it won't be evaluated until the function is called, and then it'll use its caller's value of that variable. So if a function not in the cmake subdir were to call get_patch_command, it would get the wrong directory here.
At the moment this is fine, because all the callers live in that subdir too, but I could imagine it maybe not staying that way. So for one extra safety precaution, how about copying ${CMAKE_CURRENT_LIST_DIR} into a new variable in this include file:
set(patch_list_dir ${CMAKE_CURRENT_LIST_DIR})
function(get_patch_command patch_dir patch_command_out)
set(patch_script ${patch_list_dir}/patch_repo.py)and similarly refer to ${patch_list_dir}/../patches?
There was a problem hiding this comment.
True - although setting patch_list_dir inside patch_repo.cmake does not guarantee that the value will still be correct by the time the function is called, since it can be freely modified within the scope. The safest option I can see is pass the toolchain root to the function as an argument, which can then be used within to derive the patch/script locations.
There was a problem hiding this comment.
I'm not sure what you mean by that. Yes, something else could modify patch_list_dir, but it would have to do it on purpose – I can't see any plausible way it would happen by accident, in the way that "oops I called get_patch_command from a script somewhere other than the cmake directory" seemed to me like an easy mistake.
But this version works too!
cmake/patch_repo.cmake
Outdated
| # patch_repo.py script using a target set of patches. | ||
|
|
||
| function(get_patch_command patch_dir patch_command_out) | ||
| set(patch_script ${CMAKE_CURRENT_LIST_DIR}/patch_repo.py) |
There was a problem hiding this comment.
I'm not sure what you mean by that. Yes, something else could modify patch_list_dir, but it would have to do it on purpose – I can't see any plausible way it would happen by accident, in the way that "oops I called get_patch_command from a script somewhere other than the cmake directory" seemed to me like an easy mistake.
But this version works too!
To simplify patching, the script used for the LLVM repo can be expanded for use on all repos. This also updates the newlib patch to use the same format as llvm-project and picolibc; using git's patch format means the .patch file is compatible with both git am and git apply, whereas a simple diff only works with git apply.
This also adds an option to control the patch method used by the script, since using git am may be preferable to the default git apply. The script already supports the option, so all that is needed to pass down the selection from the CMake cache if present.
Running git --check together with --apply --3way does not generatean error return code when the patches are valid but contain conflicts, so this particular combination is now marked as incompatible with the --restore_on_fail option, since this relies on --check generating an error.