-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Work around the infamous locked DLL issue #368
Comments
Here is what should happen ideally, from the user's point of view. (It might not be possible to achieve this, though.)
|
Is this right? Shouldn't we throw an error if the new package can't be moved into the library? Edit: it looks like it errors for this as well (to me) |
@jimhester yes, sry, updated above! |
Having more diagnostic info would be nice, but in general what is wrong with what pkginstall does? |
I think pkginstall is basically doing the right things. It could provide more info, and maybe even help you resolve the issue, see the edited #368 (comment) above. I should have said it at the beginning, but there are two goals here, one is having a good way to handle this in remotes/pak/renv/etc. via a standalone file, the other is to see how we could write a patch to |
@jimhester what do you think about #368 (comment) ? |
I think it sounds pretty good overall |
One question is whether we want to do sg about source installation. pkginstall always runs Working around source installation is much more difficult, because we need to call |
More background to gather in one place : |
I'm willing to help with a fix. But I don't like starting to work on something only to find that R-core are already working on it, or know of failed approaches they've tried before. I'm hoping for some kind of interaction with R-core on it before I look myself. But they haven't replied and the bug is still unconfirmed status. |
@kalibera is working on it already, apparently: |
Seems like R-devel fixes it. 17478 has been closed. |
FYI, the fixes have been ported to R-patched as well. |
Independently of that, do we want to offer a helper that checks all files under the package library if they are in use, and by what process, and offers a way to kill? |
@krlmlr Yeah, that would be useful, but it is not at all trivial to find the processes that are locking a file. |
Is this problem already fixed with R 4.0 and current version of I met this problem with
If the problem mentioned in this issue has been fixed in R and remotes, the issue I met above should not appear, right? Alternatively, at least for my case, I think some logic can be added to avoid install a package with same version again, which doesn't make sense anyway. So when user chose not to install from source, there should be some check before installing binary version, because the plan of installing this package was based on the source version is newer, but now the binary version is same and the installation should be skipped. I think this should be easy to implement as there is no locking involved, just add some check logic is enough. I'd happy to try to implement and test it. |
It seemed the timing of each choice is a little bit tricky:
The better approach might be change in install.packages side, otherwise it's hard for remotes to determine right version first without user input on binary/source, unless it's specified from the beginning as parameters. |
@dacodoc, I don't think you should try to fix this issue. |
I agree, this is not as straightforward as I thought. |
Can't you trigger/ask for a reload of the session like RStudio does? |
We could unload the packages that are loaded in the same session, before the installation. But we cannot detect the DLLs that are locked by other R processes, without the ps package or compiled code. In any case, this is now solved nicely in pak, so it is not very urgent to improve remotes: |
Unloading packages cannot be expected to work reliably (see e.g. ?dyn.unload, one of the problems are packages registering finalizers, but there are more). |
Yes, that is a risk that we take. It does work for most packages. Reloading does fail often, so pak suggests a restart after the installation:
|
As mentioned we have code in pak to help with this, and doing a similar thing in remotes would be challenging because we cannot use external packages. We don't plan on addressing this further in remotes, so I am going to close this issue. |
I also got \R\win-library\4.0\haven\libs\x64\haven.dll: Permission denied |
@hdvinod for the record, that must have been a coincidence. E.g. maybe you started a fresh R session before pressing the install/update button. I don't think that just pressing the button works in all circumstances. Surely not if other R sessions are locking the files. What surely works are:
|
Not a stupid question at all, I am sorry for not including a link: Install as
This is how it deals with the DLLs: |
This issue is currently a place that I'll dump my notes about the investigation of this problem, and also thoughts about possible workarounds.
What R does
R 3.6.0, install binary from remote repo, or from a local ZIP file
Staged installation does not matter here.
If the package is loaded in the current R session, installations is refused with a warning:
R 3.6.0, install source from remote repo, or from local tar.gz file, or from a local directory
install.packages()
call gives a warning that the installation had a non-zero exit status:The same happens if the package is loaded in another session.
If the already installed package is of the same version as the one being installed, then the load-test succeeds and you only get the warnings about the failed move and the failed copy:
R 3.6.0, non-staged install, source from repo
The end result is the same as for staged install, although the warnings are slightly different. If the load-test fails, then we only get the warning about the failed copy of the DLL file.
What pkginstall does currently
pkginstall only installs binary packages. To install a source package, needs to be
R CMD build
-t first, and then pkginstall extracts the binary, and install the extracted binary:This is the current code:
https://github.com/r-lib/pkginstall/blob/0ca61acc1028e1f8d399bbb175824219bcee12e5/R/install_binary.R#L51-L92
In a nutshell:
The text was updated successfully, but these errors were encountered: