r-lib / pak

A fresh approach to package installation
https://pak.r-lib.org
639 stars 56 forks source link

Make `lib` parameter value of lockfile_create() the same as for the lockfile_install() function e.g. .libPaths()[1] #608

Closed pascalgulikers closed 2 months ago

pascalgulikers commented 2 months ago

https://github.com/r-lib/pak/blob/67e6347f29619ed631a69d5adda6370d22f120dc/R/lockfile.R#L24

Since lockfile_create() detects already installed packages in lib and in .Library (base and recommended packages), IMO it should have the same default value for lib as lockfile_install() for consistency. The default value for lockfile_create() is NULL and the default value for lockfile_install() is .libPaths()[1] which creates unexpected results, i.e.:

PROPOSED SOLUTION: Having the lib parameter value of lockfile_create() the same as for the lockfile_install() function e.g. .libPaths()[1] will solve this as they will be marked as 'already installed' in the generated lockfile (same as is the case now for base and recommended packages). Thus an early detection of installed available packages.

What's the purpose of a lib = NULL value for lockfile_create() anyway? Since site-library installed packages should always be considered for a dependency plan (imho). And why would one not want to detect them in lockfile_create() but try to detect them in lockfile_install() with a fallback to pkgcache? It's not consistent and produces unexpected results when packages were installed before from a different source or under a different user (in pulled base images for example).

gaborcsardi commented 2 months ago

The goal of the current process is reproducibility. A

lockfile_create()
lockfile_install()

sequence should give you the same packages on every machine, irrespectively of what was already installed before.

lockfile_install() checks that the packages that are already installed satisfy the requirements specified in the lockfile. E.g. if the lockfile says to install foobar version x.y.z from CRAN, and foobar version x.y.z is already installed from CRAN, then it will not reinstall it.

If we just accepted whatever version of the package is already installed, then different computers would give you different versions of packages for the same DESCRIPTION file.

pascalgulikers commented 2 months ago

Understood, but if you use: lockfile_create(lib = .libPaths()[1]) lockfile_install(lib = .libPaths()[1]) or just lockfile_install() since the default value of lib is already .libPaths()[1], the source (e.g. CRAN) is not being checked. So those 2 functions are inconsistent.

gaborcsardi commented 2 months ago

If you use

lockfile_create(lib = .libPaths()[1])

and package foobar is required, and it is already installed (a version that satisfies the requirements coming from DESCRIPTION), then that'll go into the lockfile, so the expected source is not CRAN, but an installed package.

gaborcsardi commented 2 months ago

Thinking about this more, I don't mind exposing this argument on GHA, so I can reopen https://github.com/r-lib/actions/issues/814

But I am quite confident that the current defaults for lockfile_create() make sense, so I am going to close this issue.