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

[aronatkins]

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]

I believe this will emit a warning and return false on failure -- do we need to handle that scenario?

[aronatkins]

You're right. I had dismissed the need to handle the error because the directory is known to be writable and we are not performing a cross-file-system move, but it's probably worth checking the return value.

[toph-allen]

No major comments from a code read. Questions that came up were answered elsewhere in the code.

One conceptual question: Do we know why the description was updated when installing from the cache? 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?

[kevinushey]

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]

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]

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.