crate / crate-dbal

Doctrine Database Access Layer for Crate.IO
Apache License 2.0
16 stars 10 forks source link

Locking dependencies #115

Closed amotl closed 3 years ago

amotl commented 3 years ago

Do we really want to lock this here? Wouldn't we then miss if something break on newer library versions?

_Originally posted by @seut in https://github.com/crate/crate-dbal/pull/98#discussion_r555696865_

amotl commented 3 years ago

Hi Sebastian,

thanks for asking. As far as I know, it is common knowledge these days to actually include the lockfiles into the repository. That way, the version pinning is more solid and other subsystems like dependency caching can build upon it, like the excellent actions/cache GHA cache action [1].

I have not been aware about those details until recently but now I am an avid user of this, so we included poetry.lock and yarn.lock files throughout many repositories we are maintaining (like [2-5]) and we never looked back.

So, it obviously becomes the obligation of the package/repository maintainer to have a sound lockfile in place. Please let me know if you have further questions.

Wouldn't we then miss if something break on newer library versions?

The purpose actually is that nothing ever breaks when everyone will adhere to the semver specs, even if the package installers don't honor those lockfiles themselves. Let us have an example here from the Python world: When I designate a caret-style dependency on the click library within the project's pyproject.toml like click = "^7.1.2", the sdist packaging step will render click>=7.1.2,<8.0 to be put into the tarfile's setup.py to be uploaded to PyPI.

So, those mechanisms create a package pinning which automatically adds an upper boundary on the next major/minor revision, depending on the circumstances. That is actually a perfect DWIM implementation. I haven't investigated thoroughly with PHP/composer, but I believe it will be the same.

In turn, it will be the obligation of the package maintainer to unlock newer major/minor dependency versions to the user of the package. However, it still leaves freedom to the package installer to automatically follow updates on the minor/patch level of those where, according to semver, the API/behavior shouldn't break.

With kind regards, Andreas.

[1] https://github.com/actions/cache#whats-new [2] https://github.com/earthobservations/wetterdienst [3] https://github.com/panodata/grafana-pandas-datasource [4] https://github.com/panodata/grafana-map-panel [5] https://github.com/daq-tools/wireviz-web

amotl commented 3 years ago

Hi again,

the article "Lockfiles should be committed on all projects" [1] explains it in much better terms than I have been able to do it and sheds more light onto all the relevant details.

With kind regards, Andreas.

[1] https://classic.yarnpkg.com/blog/2016/11/24/lockfiles-for-all/

amotl commented 3 years ago

Bumped by @alexander-schranz at https://github.com/crate/crate-dbal/pull/111#issuecomment-827995882, I now believe I might have been wrong on adding composer.lock files to repositories for PHP libraries.

alexander-schranz commented 3 years ago

Just to add some more context to this topic:

As library supporting multiple PHP Versions you actually can not use composer.lock file. As a .lock file is always specific to the PHP version you are using (and installed extensions) so also something like 7.4.0 and 7.4.3 could have different dependencies (common as maybe some libraries did conflict 7.4.0 until a specific bug was fixed).

As a library should be tested always against new dependencies which are in its range a lock file should be omitted so you get a CI error as soon as possible also in the tests when a dependency maybe accidently introduced a bc break like we e.g. had in with dbal 2.13.

amotl commented 3 years ago

Thank you for sharing your insights and for taking care of this detail on behalf of #111 and https://github.com/crate/crate-pdo/pull/121 already.

amotl commented 3 years ago

Hi Alexander,

thanks again for all the contributions to make the PHP drivers compatible with PHP8. I believe it is safe to close this issue?

With kind regards, Andreas.