NotificationsYou must be signed in to change notification settings
Fork9
Star45

Test failures, fixes #21#22

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

eddelbuettel merged 3 commits intoeddelbuettel:masterfromjranke:test_failures

May 17, 2021

Merged

Test failures, fixes #21#22

eddelbuettel merged 3 commits intoeddelbuettel:masterfromjranke:test_failures

May 17, 2021

Conversation

Copy link

Contributor

jranke commentedMay 17, 2021

This PR removes some code that turned out to be unnecessary (unloading and reloading the DLL in 'moveDLL'), and which does not work with current R-devel, presumably due svn commit r80285 addressingPR#16446. The commit message contains the phrase "Zero external pointers of native symbols of unloaded DLLs", and I do observe that unloading the DLL in moveDLL removes the pointer with current R-devel. So with this commit we do not unload the old DLL any more in 'moveDLL'. The only unloading that takes place now is in case a DLL is already loaded from the target location of 'moveDLL'.

I tested this locally with R 4.0.5 and current R-devel on Linux, as well as on r-hub (Windows, Ubuntu gcc and Fedora clang), all went well. I also uploaded to winbuilder - Dirk, you have probably received the e-mail.
I also tested 'mkin' which makes use of 'moveDLL', everything appears to be fine. I am not aware that any other packages use 'moveDLL' but we may still want to do reverse dependency checks before uploading. I have never needed to do this, so I do not have a workflow for it.

jranke added2 commits

May 17, 2021 17:13

Typos, remove code that apparently did not do anything

845f1fe

At least when using R 4.0.5 on Debian bullseye (amd64), the tests alsopass without resetting the pointer at the end of 'moveDLL'.

Simplify code in moveDLL

1f43cac

in an attempt to address test failures on R-devel

Copy link

ContributorAuthor

jranke commentedMay 17, 2021

Sorry, I already bumped the version and set the date - I know this would be your domain - I just thought of that when I had already commited the updated DESCRIPTION.

Copy link

Owner

eddelbuettel commentedMay 17, 2021

Dirk, you have probably received the e-mail.

Yes, see here:#21 (comment) I actually got two. Both passes/

Copy link

Owner

eddelbuettel commentedMay 17, 2021

Sorry, I already bumped the version and set the date - I know this would be your domain - I just thought of that when I had already commited the updated DESCRIPTION.

Please just commit again and undo it for both date and version, or, if you must, set it to an intermediate value (i.e. just add a .1 and use today's date). Thanks.

Copy link

Owner

eddelbuettel commentedMay 17, 2021

Thanks a lot for working through the details -- that is really appreciated. I would simply have turned off the tests.

As I may have mentioned, 'deep down' I do not believe we actually can do this reliably, based mostly on a lot of tests with Rcpp and RInside a decade or longer. So things may have changed, but I doubt it. I expressed that way back when.e.g to thedevtools team (or I guess these days this code is inpkgload) but they are of course entitled to their views too. For me it is simply easier to start a new R process, cleanly. No side effects.

That said, I appreciate how tricky the topic is and don't mind the extension toinline / support your code here as it seems to fit your needs. And it "generally" :) has no side effects apart from e.g. testing issues such as this, but you dealt with it very prompt;y. So all good.

I'll also run some tests but it sounds like to nixed this issue. So thanks!