search

cweagans / composer-patches

Simple patches plugin for Composer
https://www.cweagans.net/project/composer-patches
BSD 3-Clause "New" or "Revised" License
1.53k stars 240 forks source link

Support and documentation on the upgrade process from 1.7.3 to 2.0.0-beta2 #559

Open joejoseph00 opened 8 months ago

joejoseph00 commented 8 months ago

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: https://github.com/cweagans/composer-patches/issues/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


System information
================================================================================
Composer version:                                                          2.7.2
PHP version:                                                            8.1.2-1ubuntu2.14

Available patchers
================================================================================
cweagans\Composer\Patcher\GitPatcher usable:                                 yes
cweagans\Composer\Patcher\GitInitPatcher usable:                             yes
cweagans\Composer\Patcher\FreeformPatcher usable:                            yes
Has usable patchers:                                                         yes

Common configuration issues
================================================================================
has plain http patch URLs:                                                    no

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.

In Patches.php line 288:

  No available patcher was able to apply patch https://www.drupal.org/files/issues/ctools-option_to_expose-2667652-3.patch to drupal/ctools

I have tried repeating steps after composer clear-cache , it did not help, got just another patch error.

cweagans commented 8 months ago

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.

joejoseph00 commented 8 months ago

I've updated the issue summary with lots more detail

joejoseph00 commented 8 months ago

I ran all of the commands in the troubleshooting guide:

composer clear-cache;

composer patches-relock; composer patches-repatch;

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
joejoseph00 commented 8 months ago

EDIT

ignore this comment

EDIT

joejoseph00 commented 8 months ago

EDIT

ignore this comment

EDIT

joejoseph00 commented 8 months ago

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 EDIT

joejoseph00 commented 8 months ago

Ok so basically the new patches ignore inheritance basically blows up our build likely because of some cascading conflict.

joejoseph00 commented 8 months ago

Ok reverting, we'll sort this out when our parent projects have their proverbial cleaning done.

cweagans commented 8 months ago

Please provide all of the information from that template. Contents of composer.json and contents of patches.lock.json are important.

joejoseph00 commented 8 months ago

patches.lock.json

Not sure how much use the composer.json will be

composer.json

joejoseph00 commented 8 months ago

here's the composer -vvv

composer_vvv_2.0.0-beta2.txt

cweagans commented 8 months ago

The composer -vvv output says that a different patch failed:

'git' -C 'html/core' apply --check --verbose -p2 '/home/j/.cache/composer/patches/8aece04cd33ab44a512227d6191ba67c47106af6ede2efdb80f84dbf46c509d6.patch'
Executing command (CWD): 'git' -C 'html/core' apply --check --verbose -p2 '/home/j/.cache/composer/patches/8aece04cd33ab44a512227d6191ba67c47106af6ede2efdb80f84dbf46c509d6.patch'
Checking patch modules/comment/comment.services.yml...
Checking patch modules/comment/src/CommentForm.php...

Checking patch modules/comment/src/CommentLazyBuilders.php...
error: while searching for:
          'title' => t('Edit'),
          'url' => $entity->toUrl('edit-form'),
        ];
      }
      if ($entity->access('create')) {
        $links['comment-reply'] = [

error: patch failed: modules/comment/src/CommentLazyBuilders.php:180
error: modules/comment/src/CommentLazyBuilders.php: patch does not apply
Checking patch modules/comment/tests/src/Functional/CommentBlockTest.php...

Hunk #3 succeeded at 100 (offset 3 lines).
Checking patch modules/comment/tests/src/Functional/CommentTestBase.php...

Hunk #1 succeeded at 186 (offset 3 lines).

Executing command (CWD): rm -rf 'html/core/.git'
Required configuration for FreeformPatcher not present. Skipping.
Downloading https://packagist.org/downloads/
Downloading https://packages.drupal.org/8/downloads
[200] https://packages.drupal.org/8/downloads
[201] https://packagist.org/downloads/

In Patches.php line 288:

  [Exception]                                                                                                                                        
  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

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 git apply. In that case, there's not much you can do other than ignoring the patches from the dependencies and then specifying your own patches in the root composer.json

olstjos commented 8 months ago

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.

olstjos commented 8 months ago

Maybe we ask ChatGPT or Grok to write some code for this.

cweagans commented 8 months ago

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.

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 -p guessing, but it is an endless source problems. In 2.x, you're going to have to be more explicit about some things. Imagine a patch that only creates files: it will succeed with pretty much any -p value, but only one of them is actually correct. Now, we default to -p1 since we're using git apply. We also have package-level defaults overridden by the plugin (https://github.com/cweagans/composer-patches/blob/main/src/Util.php) + configurable package-level depths (https://github.com/cweagans/composer-patches/blob/main/src/Plugin/Patches.php#L141-L144) + a depth param for an individual patch definition: https://docs.cweagans.net/composer-patches/usage/defining-patches/#expanded-format

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 https://github.com/cweagans/composer-patches/issues/563 to rename it.

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.

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 patch that behaves slightly differently in some cases). git apply works the same pretty much everywhere.

From a support standpoint, "Make sure git is installed" is a much easier lift than "Make sure patch is installed, but then make sure that it's this variant of patch and also it needs to be a newer release than this one specific release that added compat with git-style patches". git is also installed pretty much everywhere Composer Patches is running anyway (since Composer uses it extensively).

joejoseph00 commented 8 months ago

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: > The patch executable that Apple provides is fine.

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? https://github.com/cweagans/composer-patches/issues/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:

1) patch -p1 --dry-run < filename.patch Success or Failure? a) Success, patch -p1 < filename.patch b) Failure git apply --summary filename.patch Success or Failure ? i) Success git apply filename.patch ii) Failure patch -p0 --dry-run < filename.patch Success or Failure ? a) Success patch -p0 --dry-run < filename.patch b) Failure Throw exception with reference about filename.patch

Maybe I misunderstood something, is this the process that you call "Guessing?". I don't call it guessing, I call it a validation process.

As a target for 2.0.0, we should strive to at least match the patching capability of the 1.7.3 release of cweagans/composer-patches.

cweagans commented 8 months ago

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.

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 "fine"

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 🤷🏻‍♂️

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.

1.x currently does the opposite of this (git apply, then patch). 2.x used to have separate patchers for BSD patch and GNU patch too (see this PR: https://github.com/cweagans/composer-patches/pull/472). The plugin itself is able to try multiple Patchers, but I'm not interested in supporting patch of any variety in the core plugin. It's too much of a support headache. Composer Patches is now extensible by other Composer plugins, so somebody else can grab the patch Patcher classes out of the PR I linked before and put them in a separate plugin if they'd like.

Maybe I misunderstood something, is this the process that you call "Guessing?".

Trying different patchers is not what I was referring to. 1.x tries patch -p1, then patch -p0, then patch -p2, and finally patch -p3. In that case, we're guessing the depth param. Any of those would succeed for a patch that only adds files and that was a constant source of problems too, so now you have to be explicit about it in 2.x.

As a target for 2.0.0, we should strive to at least match the patching capability of the 1.7.3 release of cweagans/composer-patches.

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.

cancan101 commented 4 weeks ago

Is there any updates on v2 moving out of beta?