[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#337Shreeyash Pandey (bojle) 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>
f1ced90 to
b998949
Compare
|
I've implemented the detection logic for 'git apply' for now. I can implement something similar for 'git am' too. |
|
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 |
|
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. |
Jonathon Penix (jonathonpenix)
left a comment
Jonathon Penix (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.
I get what you mean about making it cleaner.
Just to expand on above a bit, I guess my hesitation about just leaving it to the script default is that I think apply is the correct option for this use case given ex: how we generate VERSION.txt, see generate_version_txt.cmake).
Hopefully it can maybe save someone from some confusion if they read "no explicit --method" as "either apply or am works here" and get confusing SHAs in their VERSION.txt.
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.
This is code we inherited from Arm--I didn't check their original commits or anything, but as I see it:
applyis helpful for our "normal" operation--we can patch the repos and check the git revision after the fact to generate VERSION.txt, etc. See generate_version_txt.cmakeamis helpful when you need to work on that repo--so ex: put the patches into the repo and create or cherry-pick more commits on top that you want to create patchfiles of, rebase the patch files, etc.
There was a problem hiding this comment.
Why this change?
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.
There was a problem hiding this comment.
In the "normal" path where it applies cleanly, there shouldn't be any error messages right (in that the reverse check wouldn't run and subsequently fail)? Seems like there wouldn't be any noise there.
Then in the "worst" case where the patch doesn't apply (so both checks would fail?) it seems like you'd want at least some of git's error messages.
Signed-off-by: Shreeyash Pandey <shrpand@qti.qualcomm.com>
There was a problem hiding this comment.
Sorry, revisiting this after a while.
I still agree that the current situation can be annoying to deal with, but I'm still not sure what the right path forward is.
Ultimately, I think changes like this are purely for developers (since like Navaneeth mentioned the buildbot builds should always be fresh) but getting this to work robustly in those cases seems tricky without knowing what people are doing?
Ex: I don't think it is unreasonable to imagine someone checking out their own repos, trying to patch them manually, and not wanting the reverse checks -> patch application to be skipped.
Maybe just making it easier to reset the repos before applying patches again would be sufficient? It might be simpler/more "safe" in that it'd be something the developer still has to do themselves, so there isn't any guesswork on our part about intent.
I'd be curious to hear your thoughts on this stuff/if you still are interested in a change like this, etc.
There was a problem hiding this comment.
In the "normal" path where it applies cleanly, there shouldn't be any error messages right (in that the reverse check wouldn't run and subsequently fail)? Seems like there wouldn't be any noise there.
Then in the "worst" case where the patch doesn't apply (so both checks would fail?) it seems like you'd want at least some of git's error messages.
There was a problem hiding this comment.
As-is, would this actually work if multiple patches are already applied that "interact" with each other at a text level?
Ex: if patch 1, 2, and 3 were applied in that order and they touch the same code (patch 3 tweaks code added in patch 2, which tweaks code added in patch 1, etc.):
- They should apply cleanly the first time around
- When we reapply, the (re-) application for patch 1 would fail as it doesn't apply
- Then we try to reverse patch 1 but I think that should fail too since patch 2/3 modified the code
Seems like we'd have to reverse from the top of the patch "stack" unless I'm misunderstanding something?
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.