[patch_repo] allow script to detect if patches have been applied already#337
[patch_repo] allow script to detect if patches have been applied already#337bojle wants to merge 2 commits into
Conversation
In the current implementation, re-running builds fails as git apply fails to apply patches to _deps/* due to the patches already being applied. This patch fixes that by detecting if patches have been applied using git apply --reverse checks. Signed-off-by: Shreeyash Pandey <shrpand@qti.qualcomm.com>
|
I've implemented the detection logic for 'git apply' for now. I can implement something similar for 'git am' too. |
|
Hi @bojle, It seems you are trying to apply the patch files over and over in your local build, not following the instructions to skip patching: https://github.com/qualcomm/cpullvm-toolchain/blob/release/qualcomm-software/22.x/qualcomm-software/docs/developing.md#manually-checking-out-and-patching-dependencies https://github.com/qualcomm/cpullvm-toolchain/blob/qualcomm-software/qualcomm-software/docs/building.md#patching No need to change the github workflow, as Navaneeth pointed out, the workflow creates a new space every time |
|
Hi @apazos Please see my other comment #337 (comment) In local workflows, any change to cmake variables will trigger the patch application script which ultimately fails if its not the first application. Only workaround so far is the cmake variable to disable it (which is a manual process), if its just a python script applying the patch, why can't it check and fail gracefully when its not the first application? |
|
The proper cmake command should include -DFETCHCONTENT_FULLY_DISCONNECTED=ON for your local build. |
jonathonpenix
left a comment
jonathonpenix
left a comment
There was a problem hiding this comment.
I agree that the current situation can be a bit painful, and I think something like this makes sense to me?
The one thing I'm unsure about is ex: if we have a patch that is essentially an "early merge" from upstream (while it is still under review, etc.) and that patch eventually comes in from upstream would this treat the patch as already applied?
I wonder if it'd be helpful to error in that case (though arguably people should keep an eye on when their patches will come in). I don't think we could actually distinguish the two cases either. I think this is only a minor concern though.
There was a problem hiding this comment.
Why remove this?
I guess it should be NFC given the current defaults, but seems unrelated to this patch.
apply is important here I think, so I don't think it is necessarily an issue to be explicit about that
There was a problem hiding this comment.
I removed it as patch_repo.py uses 'apply' as the default method, and providing --method apply is just an affirmation. Thought it might make the command line cleaner (fewer flags to remember). That said, I don't have a problem with explicitly providing --method.
There was a problem hiding this comment.
Is there a limitation that this can't happen with am?
There was a problem hiding this comment.
No limitation, I added support for just git apply as I am unsure why we have two variants to do the same thing.
There was a problem hiding this comment.
Similar to above, do we actually care about capture_output/text?
There was a problem hiding this comment.
Both can be removed, they are remnants from testing. Although, capture_output can be useful as without it, git will emit its error messages to the stdout when forward and reverse check are run (either one is bound to fail atleast once) - capture_output keeps it clean.
Signed-off-by: Shreeyash Pandey <shrpand@qti.qualcomm.com>
4 participants
In the current implementation, re-running builds fails as git apply fails to apply patches to _deps/* due to the patches already being applied. This patch fixes that by detecting if patches have been applied using git apply --reverse checks.