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

Cross platform paths #19

Merged
merged 2 commits into from
Nov 28, 2020
Merged

Conversation

jranke
Copy link
Contributor

@jranke jranke commented Nov 28, 2020

This PR fixes a test failure on windows. The commit that fixes the test is actually only the second one.

The first commit removes some complexity in compileCode. It removes handling of some corner case where R CMD SHLIB may create a DLL with extension .sl on Unix. As the DLL file name is created using tempfile(), I think removing this corner case does not hurt, the file names should be unique anyways so that the object created should always have extension .so.

BTW, I was pretty much lost in trying to fix this until I discovered the awesome little helper function cow.r!

@eddelbuettel
Copy link
Owner

so that the object created should always have extension .so.

That may not be correct. I think R itself as a function to determine the proper ending; code in R CMD SHLIB may reveal that.

The tempfile function can take a suggested ending [per my patch to R years ago ;-)] so maybe we can take that?

@eddelbuettel
Copy link
Owner

eddelbuettel commented Nov 28, 2020

Oh, and now that I look at the code I see you use that via .Platform$dynlib.ext. Excellent. This may be a minor 'smell' in use of paste():

libLFile <- file.path(tempdir(), paste0(f, .Platform$dynlib.ext))

where

libLFile <- tempfile(pattern="link", fileext=.Platform$dynlib.ext) 

may be one better. I will just make the change.

Edit Didn't work. Oh well.

R/cfunction.R Show resolved Hide resolved
@eddelbuettel eddelbuettel merged commit 538b7b9 into eddelbuettel:master Nov 28, 2020
@eddelbuettel
Copy link
Owner

eddelbuettel commented Nov 28, 2020

Looks good now. R-devel and r-release win-builder came back already with 'OK'. Let's rest the thing for a few hours and see if anything comes. Otherwise I will wrap it up as 0.3.17 tomorrow and ship.

@jranke
Copy link
Contributor Author

jranke commented Nov 28, 2020

OK, fingers crossed that MacOS will not gives us headaches...

@jranke jranke deleted the cross-platform_paths branch November 28, 2020 20:02
@eddelbuettel
Copy link
Owner

For that we have com.r and c4c.r ;-)

@jranke
Copy link
Contributor Author

jranke commented Nov 29, 2020

Ah, com.r had escaped my attention. c4c.r only checked on two Linux flavors and Windows when I tried it. Nice work.

@eddelbuettel
Copy link
Owner

:) c4c should be mac + win + linux, and if compiled code, also linux 'asan'. RHub does the dispatching, not me. You may have overlooked the mac there. It's all still imperfect in the sense of build depends possibly missing etc

@eddelbuettel
Copy link
Owner

Grrrrr. And just as I thought I had everything read, RHub (windows) gets me

  Running test_utilities.R..............    8 tests 1 fails Error in .local(x, ...) :
    Failed to copy DLL from C:\Users\USERwiEpaunUPL\AppData\Local\Temp\RtmpcxZesH/file18c06bae854.dll to C:\Users\USERwiEpaunUPL\AppData\Local\Temp\RtmpcxZesH/testname.dll
  Calls: <Anonymous> ... FUN -> eval -> eval -> moveDLL -> moveDLL -> .local
  In addition: Warning message:
  In file.create(to[okay]) :
    cannot create file 'C:\Users\USERwiEpaunUPL\AppData\Local\Temp\RtmpcxZesH/testname.dll', reason 'Permission denied'
  Execution halted

@eddelbuettel
Copy link
Owner

Adding winslash="/" to normalizePath() does nothing, this may be related to running DLLs.

 310#> Calls: ... FUN -> eval -> eval -> moveDLL -> moveDLL -> .local
 311#> Failed to copy DLL from C:\Users\USERQoAhFcvsYf\AppData\Local\Temp\RtmpmcjpbE/file278425fb5424.dll to C:/Users/USERQoAhFcvsYf/AppData/Local/Temp/RtmpmcjpbE/testname.dll
 312#> In addition: Warning message:
 313#> cannot create file 'C:/Users/USERQoAhFcvsYf/AppData/Local/Temp/RtmpmcjpbE/testname.dll', reason 'Permission denied'
 314#> Execution halted

@eddelbuettel
Copy link
Owner

So I propose to release with the following change skipping the "interesting" tests on Windows:

modified   inst/tinytest/test_utilities.R
@@ -1,5 +1,7 @@
 library(inline)
 
+onWindows <- .Platform$OS.type == "windows"
+
 code <- "
       int i;
       for (i = 0; i < *n; i++)
@@ -28,6 +30,8 @@ quadfn <- cfunction(signature(n = "integer", x = "numeric"), code,
 moveDLL(quadfn, name = "testname", directory = tempdir())
 expect_identical(quadfn(5, 1:5), res_known)
 
+if (onWindows) exit_file("Omitting remainder on Windows.")
+
 expect_error(
   moveDLL(quadfn, name = "testname", directory = tempdir()),
   "DLL .* in use")

That also means we actually don't really know if moveDLL() works on Windows. That may be a problem for you in mkin.

But should of getting help from a Windows expert (hello @aadler -- could you possibly help us?) we're stuck here.

[ @aadler In case you have a moment. This is package inline combining two of your favourites: Fortran (but also C and C++) and Windows. On Windows moving/creating a file seems to fail about 5 out of 6 times. You can run the test file in question in isolation via Rscript -e 'tinytest::run_test_file("inst/tinytest/test_utilities.R") ]

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