Skip to content
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

Test failures, fixes #21 #22

Merged
merged 3 commits into from
May 17, 2021
Merged

Conversation

jranke
Copy link
Contributor

@jranke jranke commented May 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 addressing PR#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.

At least when using R 4.0.5 on Debian bullseye (amd64), the tests also
pass without resetting the pointer at the end of 'moveDLL'.
in an attempt to address test failures on R-devel
@jranke
Copy link
Contributor Author

jranke commented May 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.

@eddelbuettel
Copy link
Owner

Dirk, you have probably received the e-mail.

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

@eddelbuettel
Copy link
Owner

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.

@eddelbuettel
Copy link
Owner

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 the devtools team (or I guess these days this code is in pkgload) 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 to inline / 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!

Copy link
Owner

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@eddelbuettel eddelbuettel merged commit c9cc61f into eddelbuettel:master May 17, 2021
@jranke jranke deleted the test_failures branch May 18, 2021 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants