Remove composer.lock from version control #4464
Conversation
|
See #4463 (comment) regarding the checks which haven't completed yet. |
|
#4463 has been merged, you can go ahead :) |
greg0ire
dismissed
their stale review
I think you either need to bump the Psalm version here:
, or make a PR to apply doctrine/.github#15 beforehand.
|
*should |
| @@ -28,7 +28,7 @@ jobs: | |||
| - "7.4" | |||
| - "8.0" | |||
| dependencies: | |||
| - "locked" | |||
| - "highest" | |||
There was a problem hiding this comment.
This way, if a dependency with an unintentional BC break gets released, whoever files their PR first after that will get their build failing although they won't have broken anything.
There was a problem hiding this comment.
Are you having this concern for all dependencies or just for the "tools"? We could use fixed versions for dependencies that are tools, that would be more explicit anyway, right now it's hard to tell if we are up-to-date or not just by looking at the composer.json. For actual dependencies such as doctrine/cache or doctrine/event-manager, the level of urgency is way higher if it breaks the DBAL, because those dependencies affect the end user unlike CS and SA libs, so I'd argue it's better for us to have a way of noticing that kind of issue early.
There was a problem hiding this comment.
My concern is about the non-tools. I agree it's good to notice those issues earlier but not in a "pipeline broken" emergency way, especially given that some of them may be false-positive.
We could use fixed versions for dependencies that are tools [...]
That would be a must to proceed with this change.
Another minor concern is that right now it's easy to switch between different branches and do composer install from the cache, while without the lock file, it will require composer update which is much slower.
There was a problem hiding this comment.
which is much slower
That's no longer true with Composer 2 is it?
My concern is about the non-tools.
In the case of the DBAL, we have doctrine/cache, doctrine/event-manager and symfony/console as a dev dependency. Given the amount of activity on the 2 first libs, I think the issue you mention is not very likely to occur, at least not very often. Not sure how much symfony/console could disrupt the build.
There was a problem hiding this comment.
That's no longer true with Composer 2 is it?
It still is. Instead of using the resolved and locked dependencies, Composer still has to update its own internal database from Packagist and resolve the constraints. And only after that, it will install the dependencies from the cache.
I think the issue you mention is not very likely to occur, at least not very often.
This is true. But I don't get it, what problem is being solved by removing the lock file? If it's for the ancient PHP versions support, then composer update can be used only for those versions (e.g. on CI). There's no point in removing the lock file entirely.
There was a problem hiding this comment.
It still is.
rm composer.lock vendor -fr && time composer update
...
composer update 1.49s user 0.79s system 84% cpu 2.713 total
This looks more than acceptable to me.
But I don't get it, what problem is being solved by removing the lock file?
- we should get fewer conflicts when merging up
- it makes the CI more straightforward (no need to install here and update there)
- using fixed versions for tools should make it easier to upgrade them individually (that being said we could already switch to fixed versions without removing
composer.lock) - if releasing a Doctrine package breaks other Doctrine packages, we might know faster and in a way that is easier to replicate with failing builds than with issues.
- it's consistent with other Doctrine packages
If it's for the ancient PHP versions support, then composer update can be used only for those versions (e.g. on CI)
Not so long ago you would have had to do this also for the CS build because doctrine/coding-standard 8 did not support PHP 7.1 anymore. With that policy, we should be able to use the latest version of each tool even if it drops a version of PHP we still want to support.
There was a problem hiding this comment.
it's consistent with other Doctrine packages
If the organization decided that this the way to go, then there's no much point in having this discussion.
There was a problem hiding this comment.
Ok 👍
@nicwortel , can you please change your PR to take this into account:
We could use fixed versions for dependencies that are tools [...]
That would be a must to proceed with this change.
This applies to all dev dependencies except symfony/console
There was a problem hiding this comment.
I was a bit occupied the last couple of days, so I haven't had the time to read this discussion until now.
I have to say that I simply decided to create this PR when I was working on #4463 and noticed that there was still a composer.lock in this repository, and this repo wasn't marked as done yet in https://github.com/orgs/doctrine/projects/7#card-50248076. I thought it would be a nice "quick fix" while I was already working on CI and Composer related stuff in this repository anyway, and didn't expect it to be such a controversial change.
To be honest, I have a little doubt whether I should proceed with this PR or perhaps I should close it until some more discussion is had within the core team. Although I personally prefer to gitignore the composer.lock of my libraries (because I want to know if a newer dependency version breaks my library as soon as possible), I don't have experience with maintaining libraries as popular as the Doctrine ones and the concern raised by @morozov sounds valid.
@morozov: perhaps it would help to have a scheduled (for example daily) build of the default branch? That way PR builds will still fail if a dependency is released with an unintended BC change, but (within the scheduled interval) the default branch would fail as well. That would at least make it clear that the build is not broken by a change in the PR but because of something else.
I do think that it is good to have a failing build if something breaks because of a broken dependency, because as @greg0ire already mentioned that would make it very visible that something is broken for the end user and some kind of action is required. Perhaps having a scheduled build will achieve this faster and without "blaming" the author of the next PR.
There was a problem hiding this comment.
To clarify my opinion on this change: I see that it solves certain problems and creates certain new problems, it's not a silver bullet. The organization believes that the problems it solves are more important than the ones it creates.
While I don't share this opinion, I'm fine with @greg0ire making this call and owning it. As long as you two believe it's the right thing to do, please go ahead.
|
@nicwortel please take a look at the PHPStan failure |
This prevents randomly failing builds in case one of the dependencies releases an unintentional BC break. See https://github.com/doctrine/dbal/pull/4464/files#r542180416 for more information.
PHPStan actually failed because I used a newer version than the one which was locked in
I think it would be good to update PHPStan to the latest version and tackle these situations, but within the scope of this PR I decided to require the previously locked version of PHPStan. |
|
Similar to #4463 (comment), some checks have to be removed from the list of required checks because they no longer exist:
|
|
…and the |
Built upon #4463, that PR should be merged first.
See https://github.com/orgs/doctrine/projects/7#card-50248076.