Add support for Composer v2 #189
Conversation
|
@tobias-kuendig could you test this with October & Composer 1.0 & 2.0? |
|
Note: I last tested these changes with Composer v1.10.15 / v2.0.2. I do am unaware if it works in latest versions. |
|
When will it release for composer 2.0 ? |
|
are looking forward to |
|
Thanks for the huge work, any ideas when it will merge. |
|
Tested with Composer 2.0.3. Works for me. Thanks! |
|
@sebschaefer how you solved that? |
|
You can check this PR in your projects by requiring I just checked this with concrete5 and this PR fixes the issue. |
|
Hello, Thanks to everyone involved in this PR. |
|
This PR worked for me too, thanks! |
|
This PR seems to work mostly correctly. (Only the first time, the next |
@prudloff-insite Thanks, I'll check this out. |
|
Also it looks like the plugin now runs |
@prudloff-insite Good point! I had a feeling there was some flaw. When the plugin is first installed, it triggers a second install / update. In Composer 2, I'm forcing an update to add the inherited dependencies to the lock file but this is clearly an issue with existing root dependencies that might not want to be updated. I'll try to look into it soon. If you have any suggestions, I'm all ears. |
|
Would it be possible to run Or at least add a flag that we can use on the CI to tell it not to update the lock file. |
src/MergePlugin.php
Outdated
| $event->getComposer()->getLocker()->isLocked() | ||
| ); | ||
| } else { | ||
| $this->state->setLocked(false); |
There was a problem hiding this comment.
Find alternative to install inherited dependencies without affecting locked dependencies.
Also it looks like the plugin now runs
composer updateafter the first install?
This is a problem when runningcomposer installon a CI.
We expect the locked dependencies to be installed as is and not everything to be updated.
Originally posted by @prudloff-insite in #189 (comment)
There was a problem hiding this comment.
This could maybe be resolved using setUpdateAllowList() with setAllowListTransitiveDependencies() in Composer 1 and setUpdateAllowTransitiveDependencies() in Composer 2.
There was a problem hiding this comment.
@prudloff-insite Can you provide me a copy of the composer.json and composer.lock that should not be updated when requiring the composer-merge-plugin?
There was a problem hiding this comment.
Here is a minimal Drupal project that reproduces our issue: https://gist.github.com/prudloff-insite/d4523328892937091b5b43f1e95919fc
The first time a contributor calls composer install with Composer 2, it will run composer update and modify the lock file.
IMHO composer install should never modify the lock file, only composer update or composer require should try to pick up new merged dependencies.
There was a problem hiding this comment.
Thanks. I'll give this a try as soon as I can.
I did some tests over the weekend and I wasn't able to reproduce the issue despite knowing why it would occur.
There was a problem hiding this comment.
I have to agree with @prudloff-insite , install should never run update. This is causing havoc in our CI as well. It is unexpectedly (and unnecessarily) updating packages without consent. Luckily this is stopped by our CI comparing locks before proceeding, but this could be a major issue for others without that stop-gap. I think the original method of checking lock is fine and it's expected that a user runs composer update locally as part of their update to composer 2 (which is also noted in the README).
Possibly even just adding a note instead to the console "This appears to be your first install, run composer update to xxx......."
|
I've implemented @prudloff-insite Can you test this out on your end? Thanks. I used the Composer files you shared with me, back in November, and things appear to be working as intended. |
|
Trying to update to composer 2.0 and plugin 1.5 in one go (still needs two No vendor repo, 1.4.1 and composer 1.x
Upgrade the plugin on 1.x, run update on 1.x, run update on 2.x (effectively a no-op) No vendor repo, 1.5, composer 2.x This LGTM, on the whole. The 1.x -> 2.x and plugin update in the same go not working is the only workflow that isn't great, but easily "fixed" on the local install by running composer again if necessary. The other install/upgrade/setup paths seems good. Is there any other way we can improve that from the composer/composer plugin side? Maybe a question for composer upstream. I'm sure there won't just be MW installs where someone might upgrade the plugin (by upgrading MW, in this case; similar upgrades for other software distributions) and composer all in one go. It's obviously a bit of an edge case though https://gerrit.wikimedia.org/r/c/mediawiki/core/+/660984 works for MediaWiki at least, and should visibly notify people that their MW install might be broken till something is remedied. |
With the latest commit, Can you provide me with your
I've left the new instruction in the README regarding updating with Composer 1 first but this section might benefit from additional details.
👍 |
As per the output, that is the version it upgraded to. But it required a second composer run to do it
I'll get the files together and post them. |
|
Ok, to be fun... There's a composer.json and composer.lock, and there's a composer.local.json which then includes 6 other composer.json files from a tree. I imagine you don't want to close numerous large repos :) So I've made a But confirmed, if I setup vendor on 1.x, |
|
@mcaskill I tried 4195d45. |
|
Oh, yeah, I should take into account that |
Fixes backwards compatibility with versions older than Composer 1.10.8. Amends 4195d45
|
This morning while updating a client project, I stumbled upon the same bug as @AntoninSlejska and @chrissnyder2337: When it occurred, the plugin was being updated from @Seldaek Sorry to bother you. Do you have any idea why there would be a discrepancy? Maybe it has something to do when |
|
Yeah Composer makes an effort to load the new version of the plugin class after an upgrade, by renaming the class in memory and loading that, but if the plugin has dependencies that are already loaded those do not get updated and there you may get trouble. This works well for plugins which are in a self-contained class. See https://github.com/composer/composer/blob/40095b20dc58772b7b630e1ca41f024fda7eae44/src/Composer/Plugin/PluginManager.php#L204-L223 for details but this won't really help you here. The solution would be to make the new code check for method existence I guess and abort if it's missing, to make the upgrade smoother. Or put all the relevant/required logic in that one class.. |
|
It looks like this class renaming issue has been there since before composer 2.0, so there's no obvious need to deal with it in the same PR. |
|
I'm thinking of adding a method exists check on |
|
I was thinking we could change the namespace to Wikimedia\Composer\Merge2. We'd increment the namespace version number every time there is a similar breaking change. |
There was a problem hiding this comment.
I've finished reviewing this. If @mcaskill is happy with it being merged now, then I'll merge it. I don't think it's blocked on a fix for the class renaming issue. If a fix for that appears in a separate PR then we can still include it in 1.5.0.
|
Thanks for merging. I'm not satisfied with the last outstanding issue of the outdated We could introduce a similar class-swapping mechanism as Composer's Instead of |
|
Sounds reasonable to me. PR welcome ;) It's probably easiest to just start a PR (if you're willing to work on this issue too), and then it's a bit easier to make a decision about naming when we have a more concrete example. Renaming/moving things around is easy enough while it's still WIP |
|
I might not be able to start work on this over the weekend. If someone else jumps on the occasion, I am available to contribute to that pull request. This is indeed turning into a major update (1.4 → 2.0) rather than a minor update (1.4 → 1.5). |
|
I don't think needing to bump the major version is a particular issue. There's some that would argue (and some that wouldn't!) that bumping the required PHP version alone does that. |
|
Of course. I'm bringing this up in relation to @tstarling proposing to rename the namespace to force Composer to load the new classes once it boots the new |
Agreed. Since this is distributed on Packagist and packagist.org states under Managing package versions: "The use of Semantic Versioning is strongly encouraged", it makes sense to use semantic versioning. SemVer says "increment the... MAJOR version when you make incompatible API changes" and since you need to use composer2 to use this update, bumping the major version makes sense. |
Unfortunately, many projects don't see it that way. |
Two of the links you provide talk about specific situations, but don't really make broader recommendations relevant to this situation. The first link is about the problems caused when including The second, Composer, semver, and underlying dependency changes, does offer some good insights into what we might do here. The post makes an attempt at offering some best practices. The third link ostensibly refutes the second link, but is really just talking about some of the problems they ran into. I think this sentence is telling:
This is another best practice, keep your dependencies up to date (which is kind of what this whole "Add support for Composer v2" is about). Best practices (what the second post offered) are not a panacea. Composer isn't perfect, but if used with best practices, suffering is reduced. But, with all this in mind and after reading the "Composer, semver, and underlying dependency changes" link, I think only a minor version bump is appropriate. |
|
FWIW I tend to agree that bumping dependencies is not a BC break. As long as your API does not change, it's not a BC break. Your dependencies are in theory not part of the API, they're an implementation detail the consumers should not have to know about. That said, with PHP version I can see that this being a dependency of all php code, including your consumers, it can make sense to treat it as a BC break too. Projects like Symfony for example do this and I find it reasonable too, it's much easier to communicate to users for starters, and it's part of their BC promise, which is an extension of semver. The main benefit I see of bumping to 2.0 here would be that it leaves the door open to releasing a new 1.5 for new features for composer 1, but if you don't see yourselves doing that then IMO leaving it at 1.5 now makes more sense, and it simplifies the upgrade path as people requiring the merge plugin with |
|
I think people have lost track of why the "1.5 vs 2.0" proposal. Me and @tstarling are referring to renaming the plugin's PHP namespace to prevent MergePlugin 1.5/2.0 from using the, potentially already autoloaded,
Such namespacing implies we bump the package's version to Alternatively, maybe we can rename |
This is OK now, because before Composer v1 support was required for wikimedia/composer-merge-plugin - and now the plugin supports Composer v2. wikimedia/composer-merge-plugin#189
Latest Update 2021-01-25 14:25 EST
Notes:
See comment.
See comment.
See comment.
composer updateagain.See comment.
isFirstInstall()on Composer v2. Listening only topost-update-cmdinstead ofpost-install-cmdmight be more sensible.See comment and followup.
How to test:
{ "repositories": [ { "type":"vcs", "url":"https://github.com/mcaskill/composer-merge-plugin" } ], "require": { "wikimedia/composer-merge-plugin": "dev-feature/composer-v2 as 1.5.0" } }Tasks:
See comment #1, comment #2, comment #3.
MultiConstraintto latest version of composer/semver.See comment.
Factory::getLockFile()does not exist in Composer 1.See comment.
Replaces #187, #185, as fix for #184 (as per composer/composer#8726)
I've tested using the example and the unit tests and it works in Composer v1 and v2.
This alternative proposal forgoes
PRE_DEPENDENCIES_SOLVINGentirely to avoid over-complicating the codebase and streamline the plugin's merge logic.The issue with the previous attempts (as well as attempts on other plugins) stems from a miscommunication from the Composer team of the major difference in how v1 and v2 resolve and install dependencies.
Back to my proposal.
Currently, the use of
PRE_DEPENDENCIES_SOLVINGin the plugin is used to inject duplicate requirements (usually with a different version constraints) into the solver (while distinguishing betweenrequireandrequire-dev).What I propose replaces the duplicate links tracking in
ExtraPackageby back porting Composer 2's new staticMultiConstraint::create()method which can be used to resolve complex-constraints early on. In turn, this makes the need forPRE_DEPENDENCIES_SOLVINGobsolete.I hope this PR will, at the very least, help to figure out how to support Composer v2.