search

rstudio / packrat

Packrat is a dependency management system for R
http://rstudio.github.io/packrat/
401 stars 89 forks source link

installPkg: do no work when using cached package and annotate safely #726

Closed aronatkins closed 9 months ago

aronatkins commented 9 months ago

First, the installPkg() function no longer does additional work when the package was discovered in the cache. This avoids the most common situation where multiple processes performing package installation can race to the annotatePkgDesc() and moveInstalledPackageToCache() calls.

Second, annotatePkgDesc() writes its update to a temporary file before renaming to the target DESCRIPTION. This avoids a (now more rare) situation where a second process might see an incompletely written DESCRIPTION file.

As a side-effect, only the originally installing actor will annotate the DESCRIPTION. Previously, an update occurred even when the package was used from the cache, which overwrote an existing annotation.

Update: After thinking about this problem a bit more, realized that the root cause to #720 is because annotatePkgDesc() was previously writing into the package cache. After calling restoreWithCopyFromCache(), the library contains a symlink into the cache. Calling annotatePkgDesc() reads through the symlink and modifies the in-cache DESCRIPTION. This is why we saw multiple actors reading/writing the same file.

Fixes #720

kevinushey commented 9 months ago

Are there any times where un-annotated descriptions can enter the cache (e.g. a package was installed via install.packages without Packrat) where we'd want to add the annotation?

A long time ago, Packrat tried to distinguish between packages which were installed by Packrat itself, versus packages that were installed by the user. The intention here was to have a way of differentiating between packages that were "managed" or "known" to Packrat, and use that to customize the UI presented to the user in certain scenarios (e.g. helping them choose whether to snapshot or restore).

In practice, users found the behavior hard to understand, so that was phased out over time. I'm not sure if we still make use of the InstallAgent annotations anywhere in the Packrat sources, but if we do, it'd be nice to drop those once and for all.

aronatkins commented 9 months ago

I'm not sure if we still make use of the InstallAgent annotations anywhere in the Packrat sources

We do still use InstallAgent. See calls to installedByPackrat().

aronatkins commented 9 months ago

Are there any times where un-annotated descriptions can enter the cache (e.g. a package was installed via install.packages without Packrat) where we'd want to add the annotation? And if so, would the atomic updates (via renaming a temp file) be sufficient to fix the problem?

Users do not enroll packages in the cache; only Packrat performs cache additions and cache-uses while installing a package. I suppose it's possible that a user could add something to the Packrat library, but that is separate from cache management.