-
Notifications
You must be signed in to change notification settings - Fork 941
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
Standarize on patch workflow/plugins #14
Comments
Or is anyone aware of other solutions? |
I have to admit that I don't use the patch workflow much. If I have to patch a contrib module, I take it out of the composer.json / makefile, and check the whole thing in to git (sites/all/modules/contrib, sites/all/modules/custom, sites/all/modules/modified, with the last two under revision control). I considered a patch workflow some time ago with kitten-o-matic, but decided against it. If I patched more, I'd probably adopt a patch workflow. With that disclaimer in mind, here are my thoughts. There seems to be two lines of reasoning here:
I tend to think that I would favor #2. In either event, nothing changes until composer update time. If something breaks, then you're going to have to re-roll the patch regardless. It's extra work to tie the version of the module to the patch, and I don't really see the value of keeping that information around (the patch for the older version of the module, once you've updated to the newer version of the module) in the composer.json file, as you can always roll back to it in the vcs. So, keep the composer.json lighter. There may be other aspects of these different solutions that I am not considering, though, as I have not evaluated them deeply. |
Nice writeup @greg-1-anderson . 👍 for option 2 as well from me.
|
I wasn't aware of jpstacey's and webflo's solutions. As mentioned in a small discussion on gdo I'm also for option 2, maybe with adding an optional version constraint. The generell problem with a patches plugin is, that you have to know if a patch was already applied or not. I'm currently not sure if there is a way to store that information in the composer.lock or the installed.json. Due to that problem |
I'm more than happy to deprecate mine in favour of the other too. It was a POC at a time when I couldn't find other resources out there. In fact, as composer's own APIs for event handling seemed to have changed last time I checked (some methods no longer available on Event), and as I don't have a client to sponsor updating it, I've already done so in favour of netresearch: but if the preference goes to webflo, let me know and I'll change the relevant notices. |
I am trying webflo/composer-patcher, and am still having a hard time getting any patch applied - still investigating. However, I would think that running on post-package-install means patches only get applied if the corresponding package/module is downloaded. In a typical workflow, adding a new patch for a module should mean the module gets re-downloaded and all its patches get re-applied on the fresh download ? Didn't check yet how netresearch/composer-patches-plugin handles that. |
@yched Your analysis is corrects. Thats indeed a problem because there is no composer event for already installed projects. Maybe we should add an additional command to force the patch apply on already existing modules? I think the netresearch/composer-patches-plugin handles this use-case because it leverages the composer dependency resolution. I just started webflo/composer-patcher because it think the config for netresearch/composer-patches-plugin is to verbose. |
I think it wouldn't be too hard to extend cweagans/composer-patches to do this. I'll probably work on this today because we need it at work. |
@cweagans oh, awesome :-) I wasn't aware of that other "composer patch" option. Here are a couple thoughts after having tried netresearch/composer-patches-plugin: As discussed above, its behavior of reapplying patches if the patch definitions changes, instead of when the patched module changes, is a must-have IMO. Being able to add a patch for a module that is already present feels pretty important ? It works that way because it defines a set of patches as a package definition, so changes in the patch set can be caught (well, provided you manually bump the package "version" on each change, which is not too great, not sure there's another way) On the minus side :
|
Also, re-downloading the module when its associated patch set changes means you need separate "patch packages" per module, so that you don't re-download modules whose patch set hasn't changed. |
So I looked into this, and it's going to be kind of a pain in the ass. Would a I was thinking that |
IMO having the ability to force a re-download of a given package would be instrumental, yes. If we have that, I think we can come up we various ways of handling a "track and apply a set of patches" workflow :
Having it all run through plugins within "composer install/update" seems nicer though ? |
As I wrote above, I think being able to do that is needed, but in the end the overall workflow has to automatically determine wich modules/packages need to be re-downloaded & re-patched, it can't be left to the user to figure out. For the use case of either a developer working in a team and pulling updates from teammates, or deploying new code to a staging/prod server, the workflow would be :
? |
I think we can integrate with the standard For further reference: https://getcomposer.org/doc/articles/scripts.md#command-events |
The problem is that if the package already exists in the file system, Using post-install-cmd means that patching things is significantly more work, too. You have to track which patches have been applied and in what order or things start breaking. I reject the notion that composer must be aware of the list of patches. As a developer, if I add a patch to a project, it's not unreasonable to expect me to run |
I know post-install-cmd is significantly more work, thats why i tried to avoid it in the first place, but we have to keep track of installed packages and patches. Otherwise we run in a lot of nasty problems with ci systems. |
Doesn't your ci command re-install everything from scratch? If so, it shouldn't be a problem, because every module will be installed and patched when you update your patch info in composer.json. The use-case we have to be particularly sensitive to is team environments, where one developer adds a patch and commits composer.json, and then the other teams check out composer.json from the vcs. It is not reasonable to expect team members to run extraordinary commands (e.g. composer reinstall on specific modules names) when pulling modules. In the current state of affairs, you could stipulate that the procedure was to remove the vendor directory every time you update composer.json. It would be way nicer to support an incremental development workflow, as composer usually allows, but as was previously observed, it's difficult / complicated to re-patch if the module being patched isn't pristine. Maybe a pre-install hook could be used to iterate over the patch information, and just blow away the on-disk representation of every module that has patches on it. If you cache the patch information when you apply patches, then you could further optimize this idea to only blow away modules when the patch information changes. It is my presupposition that if a module is removed from disk (deleted from the 'vendor' directory) during some early hook, then composer should just figure out that it's gone and re-download it. I haven't tried it, but I suspect it would work just fine. If it does, you wouldn't even need to change the existing hook implementations; you'd just need to add the patch caching, and the pre-hook that deletes targets when patch info changes. |
@greg-1-anderson That's a good idea. I'll give that a go right now. |
|
+1 to @greg-1-anderson : a deployment script is "do whatever is needed, don't ask me what". That's what git pull does for repo files, composer install for packages, drush updb for updates, config sync for config. Should be the same for patches (ideally as be part of composer install). You shouldn't have to say to your X teammates "don't forget to run 'some_command some_module' next time you pull changes locally or deploy on prod, or stuff breaks". Also, yes, being able to operate incrementally makes a huge difference, you can pull changes from teammates 10 times a day during dev, re-downloading from scratch everytime makes things painfully slow :-). @cweagans : awesome, I'll take cweagans/composer-patches 1.2.0 for a spin asap ! |
cweagans/composer-patches seems to work great ! (only on "composer install" atm though, not "composer update" - not sure if there is a reason for that ?) I created a couple PRs, most notably cweagans/composer-patches#7, which uses installed.json to track applied patches and only wipe/redownload/repatch a package if needed. |
cweagans/composer-patches now seems to have everything we need in terms of patching flow So how do we close this issue ?
Also, in issue #25 I suggested grouping "recommended drupal/composer tool" under the drupal-compooser vendor. What do you folks think of that ? @cweagans, is that something you'd be willing to consider ? |
Personally, I'd rather keep it under my username. It's useful to other projects aside from Drupal, so having it under the drupal-composer vendor might discourage people from using it if they aren't using Drupal. |
I'm struggling to get netresearch/composer-patches-plugin working Here's my patches package
Then requiring that:
|
@timmillwood composer-patches-plugin simply didn't work for me, which is why I wrote my own plugin. Try cweagans/composer-patches. It's much easier to get working. |
I updated the Composer in relation to Drush Make" doc page to recommend cweagans/composer-patches as the standard patch manager. cweagans/composer-patches works so much better than the others that I did not maintain the list of alternatives. |
People here may want to be aware that I just added an option to have an external patches file. Might be useful for some of you with a ton of patches to your projects! Glad my plugin is helpful. Happy to add features if there's anything lacking. Just open an issue and I'll see what I can do. |
Wow. Patching subtree splits is a pandora box: https://www.drupal.org/node/2606840 |
Patching subtree splits seems like an edge case. It's not quite a pandora's box; the process is straightforward, although hard. I think that the most reasonable policy here is to simply say "don't do that", and test these sorts of patches with a non-composer-managed Drupal install. If this became a common use case, it would be entirely possible to write a patch split tool. |
👍 exactly. I don't have much interest in supporting that in the patches plugin, either. If you're doing a subtree split of a package, I don't think it's unreasonable to ask that you also supply a patch that works properly with your subset of files. In my case, I'm using my plugin to apply patches to the
If somebody does this, let me know. I'll link to it from the composer-patches repo readme. |
Requiring a manual edit to split the patch and then keeping it local seems reasonable. @cweagans : is the patch plugin able to apply local patch files though ? Last time I checked (granted, that was a couple months ago) it didn't ? |
@tobiasbaehr asked me the same question at our local user group meeting yesterday. |
@yched @cweagans PR is ready. cweagans/composer-patches#22 |
We have a few different composer plugins to apply patches files after a package is installed.
I forked jpstaceys repos because i think its very convenient to manage all patches in the root composer.json file. And because of our own packagist we dont have the package definition in our composer.json. This means, we need to store this information somewhere else.
// cc @kasperg, @derhasi, @greg-1-anderson, @frega, @jpstacey: Whats your opinion?
The text was updated successfully, but these errors were encountered: