remove overwrite arg from moveInstalledPackageToCache#757
Conversation
| # | ||
| # If the package already exists inside the cache, overwrite=TRUE causes | ||
| # replacement of the cached content while overwrite=FALSE with fatal=FALSE | ||
| # uses the cached package. Using overwrite=TRUE with fatal=TRUE will err. |
There was a problem hiding this comment.
just a note because it threw me off briefly, I believe this comment was inaccurate and the combination that errors is fatal = TRUE and overwrite = FALSE
There was a problem hiding this comment.
Thanks. This comment was either incorrect when originally written or grew stale as the implementation evolved.
Should we remove fatal as well?
There was a problem hiding this comment.
Also: overwrite was a lie. Maybe always?
There was a problem hiding this comment.
sure I'll remove fatal too
| # | ||
| # If the package already exists inside the cache, overwrite=TRUE causes | ||
| # replacement of the cached content while overwrite=FALSE with fatal=FALSE | ||
| # uses the cached package. Using overwrite=TRUE with fatal=TRUE will err. |
There was a problem hiding this comment.
Thanks. This comment was either incorrect when originally written or grew stale as the implementation evolved.
Should we remove fatal as well?
| ) | ||
| } | ||
| on.exit(unlink(backupPackagePath, recursive = TRUE), add = TRUE) | ||
| return(symlinkPackageToCache(packagePath, cachedPackagePath)) |
There was a problem hiding this comment.
There are a number of paths through moveInstalledPackageToCache which discard the just-built package. Should we add logging for these situations? The computed type in installPkg is also misleading.
built source but i lost the race. discarded the build and symlinked cache.
Too wordy, but saying something more could help subsequent debugging.
There was a problem hiding this comment.
I updated to take the cacheCopyStatus argument that other related functions use, so that we can report the right info back to installPkg. Added some messaging as well.
| # | ||
| # If the package already exists inside the cache, overwrite=TRUE causes | ||
| # replacement of the cached content while overwrite=FALSE with fatal=FALSE | ||
| # uses the cached package. Using overwrite=TRUE with fatal=TRUE will err. |
There was a problem hiding this comment.
Also: overwrite was a lie. Maybe always?
There was a problem hiding this comment.
Is it worth enumerating the reasons why the rename might fail? Other than OS errors (full disk, no permissions):
- library and cache are on different file systems.
- cached location exists (created by another)
|
lintr released today and is objecting to |
😭 Thanks. They haven't published updated release notes, yet. https://github.com/r-lib/lintr/blob/main/NEWS.md#lintr-340 |
2 participants
See #756 (comment). That comment flagged that
moveInstalledPackageToCache's backup/overwrite path (renaming an existing cache entry aside before replacing it) can race with another process populating that same entry. This function is called in two places ininstallPkgand neither passesfatal = TRUE. This path was only reachable iffatal = TRUEandoverwrite = TRUEso removing it should be safe avoids a potential trap in the future.