-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
Support and documentation on the upgrade process from 1.7.3 to 2.0.0-beta2 #559
Comments
I need to know a lot more about your environment. Sorry I didn't specify, but can you provide all of the information requested by the reproducible bug issue form? https://github.com/cweagans/composer-patches/issues/new?assignees=&labels=bug&projects=&template=bug.yml You can put all the info here or you can close this and open a new issue -- whatever is easier for you. |
I've updated the issue summary with lots more detail |
I ran all of the commands in the troubleshooting guide:
In Patches.php line 288: No available patcher was able to apply patch https://www.drupal.org/files/issues/2021-06-02/comment_form_redirect-2559833-87.patch to drupal/core |
EDIT ignore this comment EDIT |
1 similar comment
EDIT ignore this comment EDIT |
EDIT ignore this comment hmm, but no, I see no PATCHES.txt in the cweagans/composer-patches folder it'self, so perhaps this patch is not a cweagans patch |
Ok so basically the new patches ignore inheritance basically blows up our build likely because of some cascading conflict. |
Ok reverting, we'll sort this out when our parent projects have their proverbial cleaning done. |
Please provide all of the information from that template. Contents of composer.json and contents of patches.lock.json are important. |
Not sure how much use the composer.json will be |
here's the composer -vvv |
The
If you look near the start of the pasted output there, you'll see that git is actually what is failing to apply your patch. It's possible that those patches worked okay with GNU Patch but not |
I'm not sure why php developers hesitate so much to do nested try catch blocks for operations that have multiple fail points. This sort of exception should be caught and we should try to find a solution, perhaps it's using patch, or perhaps it's a git apply flag option. In fact, a try catch block isn't even necessarily needed. Any pragmatic approach is fine for me with or without try-catch. It's possible to get handle error codes from the exec command. I'm not sure what error code --dry-run gives but there's more than one way to get it done. Suggested process: keep your git apply default but do a dry-run first (assuming git apply does dry-runs) then if the dry-run is successful use git apply, if not, try patch with p flag option 1, if that works, use it, if not, try p flag option 0, if that works, use it, if git apply does not have a dry-run option then use patch instead. Catch the error codes from the dry-run or whatever feedback mechanism can be used. So repeating this again: A possible solution, and I'm not that familliar with git apply but:
I know there was issues with macs using the default mac patch executable that doesn't support renaming files, most patches do not rename files, with that said, dry runs aren't that expensive. I find that it's unfortunate if we just punish Ubuntu users because Apple is not providing a proper version of patch executable by default. This could be made to be absolutely bulletproof / idiotproof and for just about everyone. |
Maybe we ask ChatGPT or Grok to write some code for this. |
We already do a dry run before attempting to apply the patch: https://github.com/cweagans/composer-patches/blob/main/src/Patcher/GitPatcher.php 1.x has Explicit configuration > random guesses. I'm completely disinterested in re-adding the guessing functionality you outlined to 2.x, but you can add it in a third party plugin that extends Composer Patches. Composer Patches emits an event when determining patch depth that allows you to programmatically determine the depth. The event isn't named correctly right now, but I opened #563 to rename it.
The patch executable that Apple provides is fine. It's just not the same variant of patch that is installed on most Linux systems. Apple ships BSD patch and most linux systems ship GNU patch. There are several other variants that exist too (e.g. busybox has their own From a support standpoint, "Make sure |
Hi cweagans, please do not take this personally, I have a high respect for your work and appreciate your efforts. I could perhaps sponsor your work if there's a paypal account somewhere that you want me to send funds to. The crux of this issue is, many of the patches we're using no longer apply when using 2.0.0-beta2. Perhaps as people discover this there will be more attention given so I imagine a pull request will come forward at some point. I'm wondering what that pull-request might look like? In response to your comment about BSD patch: I disagree, BSD patch isn't "fine". The version of bsd patch commonly installed on Apple devices cannot rename/move files, instead it throws an error code, that is why it is inferior to gnu patch. Have you not ever seen a patch rename/move files? #326 Perhaps instead of the process I am suggesting, perhaps we parse the version of patch for GNU/gnu and if GNU/gnu is not found, default to git apply only after validating for GNU patch. If GNU patch fails, could then try git apply. Keep in mind, I'm ignorant of this project code for this library. GNU patch and git apply are able to move files. With that said, the way I see it is this workflow:
Maybe I misunderstood something, is this the process that you call "Guessing?". As a target for 2.0.0, we should strive to at least match the patching capability of the 1.7.3 release of |
No worries at all -- just providing information :) Sponsorship isn't necessary, but if you feel strongly about sponsorship, my GitHub Sponsors profile is here: https://github.com/sponsors/cweagans
BSD patch isn't great, but if you know that it's BSD patch and you're not expecting GNU patch functionality, it does the job 🤷🏻♂️
1.x currently does the opposite of this (
Trying different patchers is not what I was referring to. 1.x tries
I can see why you'd make this assertion, but I disagree. 1.x was not necessarily correct and in fact, it's far more likely that 1.x was incorrect. It will probably take some work to get a project upgraded to 2.x - it's not going to Just Work, but when you get it working, you won't have to fiddle with it anymore. Maybe I'll do a screencast or something and show how to upgrade a project, how to debug stuff, etc. That might make some good documentation. I'd also be interested in poking at your project specifically. If you'd be willing to give me read access to the necessary repo(s) directly, I can take a look. |
Is there any updates on v2 moving out of beta? |
Verification
What were you trying to do (and why)?
Trying to upgrade from 1.7.3 to 2.0.0-beta2 to test it out.
Which file do you have feedback about? (leave blank if not applicable)
No response
Feedback
Spun from: #521
Question, is there documentation to assist with the upgrade from 1.7.3 to 2.0.0-beta2 , am I missing something? We're seeing patch errors.
Environment:
Reporting from:
composer patches-doctor
Overridding composer 1.7.3 because our parent project is locked on 1.7.3
composer require cweagans/composer-patches:'2.0.0-beta2 as 1.7.3';
Just hit this with 2.0.0-beta2, not sure if I understand the implications of what was said in the existing related issue.
I have tried repeating steps after
composer clear-cache
, it did not help, got just another patch error.The text was updated successfully, but these errors were encountered: