Skip to content
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

Only have git patchers and freeform patcher? #472

Merged
merged 6 commits into from
Feb 13, 2023
Merged

Conversation

cweagans
Copy link
Owner

Description

Continuation of #471. If the Git patcher works for everything, why even bother with patch? We could potentially use the git patchers for everything and people can use the FreeformPatcher that this PR adds to run arbitrary commands to apply other patches in some way.

I'm not sure if this is the right thing to do though.

Related tasks

  • Documentation has been updated if applicable
  • Tests have been added
  • Does not break backwards compatibility OR a BC break has been discussed in the related issue(s).

Other notes

@cweagans cweagans mentioned this pull request Feb 13, 2023
3 tasks
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4159058115

  • 41 of 50 (82.0%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+1.3%) to 87.413%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Patcher/FreeformPatcher.php 19 28 67.86%
Files with Coverage Reduction New Missed Lines %
src/Patcher.php 1 88.37%
Totals Coverage Status
Change from base Build 4155475134: 1.3%
Covered Lines: 500
Relevant Lines: 572

💛 - Coveralls

@cweagans cweagans mentioned this pull request Feb 13, 2023
31 tasks
@cweagans cweagans merged commit 96146bb into main Feb 13, 2023
@cweagans cweagans deleted the remove-other-patchers branch February 13, 2023 04:38
@danepowell
Copy link
Collaborator

danepowell commented Feb 13, 2023

I tested this on our project, it worked the first install (possibly because the library already existed on disk) but after rm -rf vendor && composer install I got this error:

No available patcher was able to apply patch https://patch-diff.githubusercontent.com/raw/consolidation/self-update/pull/21.patch to consolidation/self-update

I'm not sure what that error means exactly. Is there a problem with the patch? It would be pretty bad if patches directly from GitHub can't be applied.

Let me know if you want a separate issue for this.

@cweagans
Copy link
Owner Author

@danepowell I cloned your project and gave it a try with composer install -vvv. It looks like the patch is referring to a path that doesn't exist (src/Symfony/Component/Filesystem/Filesystem.php) in vendor/symfony/filesystem. main doesn't guess patch depth anymore, so you have to be explicit about what you want. You can either:

  • Change the patch to be relative to the root of vendor/symfony/filesystem
  • Change your patch definition to include the depth for that specific patch:
"extra": {
        "patches": {
            "symfony/filesystem": [
                {
                    "description": "patch does something",
                    "url":  "assets/symfony-fs-mirror.patch",
                    "depth": 5
                }
            ],
        },
}
  • Set the depth for the package (as you had done previously -- the config lives in a different place now):
    "extra": {
        "patches": {
            "symfony/filesystem": [
                "assets/symfony-fs-mirror.patch"
            ],
        },
        "composer-patches": {
            "package-depths": {
                "symfony/filesystem": 5
            }
        }
    },

After getting that patch to apply, it looks like there is another that is failing -- https://patch-diff.githubusercontent.com/raw/consolidation/self-update/pull/21.patch

This one (I think) is a legit failure. That patch was added to your composer.json 5/6/22. The version that composer is downloading includes consolidation/self-update@735ea26, which changes SelfUpdateCommand.php inside of the context that the patch provides, so it doesn't cleanly apply. I'm not seeing a way to tell git to tolerate this, but maybe updating consolidation/self-update#21 would solve that?

I didn't go further than that -- does that help?

@danepowell
Copy link
Collaborator

Thanks. For the symfony/filesystem patch, I tried setting the package depth both ways you suggested and no matter what Composer Patches passed -p1 to Git and the patch failed. I pushed a new commit so you can see: acquia/cli@a4d24c3

I'll concede that the consolidation/self-update patch may be borked.

Also this was hard to debug because the errors changed every time I ran composer install until they finally went away, but patching was still failing. Removing patches.lock didn't force Composer Patches to re-attempt patching, I had to rm -rf vendor for that. I think if patching fails, it should fail the same way every time you run composer install.

@danepowell
Copy link
Collaborator

Oh I figured it out, Composer Patches was completely ignoring my composer.json changes because patches.lock existed. In the end, removing all of my composer, vendor, and patches.lock files and setting the depth correctly fixed it. But yikes, it shouldn't be that complicated to debug.

Maybe you should store a hash of composer.json inside patches.lock so that you can make sure to pick up changes to composer.json. This is what composer.lock obviously does, and composer.lock and patches.lock are effectively siblings (I understand you created patches.lock just because you couldn't store arbitrary data in composer.lock).

@cweagans
Copy link
Owner Author

The plugin works a little differently now -- https://docs.cweagans.net/composer-patches/usage/recommended-workflows/

Not automatically deleting stuff was an explicit goal -- that caused a lot of problems for people in the past. I agree that it should have warned you that patches.lock should be updated though. Opened #479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants