Skip to content

[R-pkg-devel] Error in CHECK caused by dev.off()

20 messages · Serguei Sokol, Jeff Newmiller, Duncan Murdoch +6 more

Original
Helmut Schütz
# 
Dear all,

I have two variables, foo and bar. The first is TRUE if a png should be 
created and the second is TRUE if an already existing one should be 
overwritten.
At the end of the plot I had
if (foo | (foo & bar)) dev.off()
This worked as expected in all versions of my package built in R up to 
v3.6.3. However, when I CHECK the package in v4.0.2 I get:
 > grDevices::dev.off()
Error in grDevices::dev.off() :
 ? cannot shut down device 1 (the null device)
Execution halted

I tried:
if (foo | (foo & bar)) {
 ? dev <- dev.list()
 ? if (!is.null(dev)) {
 ??? if (dev == 2) invisible(dev.off())
 ? }
}
without success (same error).

Even the more general
if (foo | (foo & bar)) {
 ? graphics.off()
}
did not work.

The plot is called only in an example of one man-page -- though embedded 
in \donttest{}.
Even if I set both foo and bar to FALSE (i.e., the respective part of 
the code should not be executed at all), I get the same error.

Any ideas/suggestions?

Regards,
Helmut
Serguei Sokol
# 
Le 22/07/2020 ? 14:36, Helmut Sch?tz a ?crit?:
Hmm... I see 2 possibilities for still getting an error while the 
concerned part of code is not supposed to be run:

  - either you are running not updated version of your package;
  - or the error comes from some other place of the code.

Sorry but without a minimal reproducible example I cannot help more.
Best,
Serguei.
Jeff Newmiller
# 
I suspect your foo and bar variables are not logical anymore... insufficient info. However, why aren't you using short-circuit && and || operators?
On July 22, 2020 5:36:06 AM PDT, "Helmut Sch?tz" <helmut.schuetz at bebac.at> wrote:

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      Duncan Murdoch
    

    

      
      #
    

  


  

      
On 22/07/2020 8:36 a.m., Helmut Sch?tz wrote:

            

      
      
      
    

        
Assuming that foo and bar are each length one variables, this test is 
logically equivalent to

   if (foo) {

Is that really what you intended?

Duncan Murdoch

            

      
      
      
    

  
  


  


      

    

    

      
      

        


  

        

      Helmut Schütz
    

    

      
      #
    

  


  

      
Hi Serguei,

Serguei Sokol wrote on 2020-07-22 15:51:

            

      
      
      
    

        
I _can_ built the package and it runs as intended. Only the CHECK throws 
the error.

            

      
      
      
    

        
Closing the device is required only _once_ in the entire package.
In my NAMESPACE I have (and had in all previous versions):
importFrom(grDevices, png, graphics.off, dev.list, dev.off)

            

      
      
      
    

        
The problem is that I cannot reproduce it as well. Only CHECK laments 
about dev.off() which I changed to graphics.off() in the meantime.

library(grDevices)
foo <- TRUE?? # shall we plot?
png.path <- "~/" # user's home folder
png.path <- normalizePath(png.path)
if (foo) {
 ? png(paste0(png.path, "test.png"), width = 480, height = 480, 
pointsize = 12)
}
plot(x = 0:1, y = 0:1, type = "l", xlab = "x", ylab = "y")
if (foo) {
 ? graphics.off()
}

Best,
Helmut

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      Duncan Murdoch
    

    

      
      #
    

  


  

      
On 22/07/2020 1:25 p.m., Helmut Sch?tz wrote:

            

      
      
      
    

        
You don't test whether the call to png() succeeded.  During a check, it 
probably wouldn't, because you aren't allowed to write to "~/".  Your 
package should be writing to tempdir(), or a location entered by the user.

Duncan Murdoch

  
  


  


      

    

    

      
      

        


  

        

      Helmut Schütz
    

    

      
      #
    

  


  

      
Duncan Murdoch wrote on 2020-07-22 21:42:

            

      
      
      
    

        
Correct. However,
 ? if (file.exists(paste0(png.path, "test.png"))) graphics.off()
worked in the example but not in the package...

            

      
      
      
    

        
The user is asked to provide a certain path indeed. Only if none is 
provided, the fallback is "~/" (which is always writable). The package 
is intended for "common" users and not "R-geeks". If I would write to 
tempdir(), I guess chances are pretty low that a user will be able to 
locate the file.
What I still fail to understand: CHECK laments about 
grDevices::dev.off() in a certain man page, where I removed the argument 
foo completely in one example (which is within \donttest{}). Hence, the 
entire plotting routine is not executed at all. Furthermore, dev.off() 
is nowhere used, only graphics.off() - now after file.exists().

Regards,
Helmut

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      Duncan Murdoch
    

    

      
      #
    

  


  

      
On 22/07/2020 5:40 p.m., Helmut Sch?tz wrote:

            

      
      
      
    

        
That disqualifies your package from inclusion on CRAN.  If no 
destination is provided and tempdir() isn't acceptable, you shouldn't 
write anything.  The user may be keeping an irreplaceable piece of 
information in "~/test.png", and your package would destroy it.  It's 
not your decision to make to trespass on the user's file space.

Duncan Murdoch


The package

            

      
      
      
    

  
  


  


      

    

    

      
      

        


  

        

      Helmut Schütz
    

    

      
      #
    

  


  

      
Dear Duncan,

Duncan Murdoch wrote on 2020-07-22 23:48:

            

      
      
      
    

        
Can you please point to such a policy in the R-Extension Manual? Eight 
versions of the package were accepted by CRAN and three times checked by 
members of the team.

BTW, the package passed on winbuilder:
Your package replicateBE_1.0.14.9000.tar.gz has been built (if working) 
and checked for Windows.
Please check the log files and (if working) the binary package at:
https://win-builder.r-project.org/k2tGfNoY7y88
The files will be removed after roughly 72 hours.
Installation time in seconds: 41
Check time in seconds: 251
Status: 1 NOTE
R version 4.0.2 (2020-06-22)
Hence, I suspect that there is a problem with CHECK which I run locally 
on my machine.

            

      
      
      
    

        
The package is used in a regulated environment (e.g., for the FDA). The 
name of the file is always unique, i.e., depends on the input. The code 
checks whether a file with the same name already exist and -- if yes -- 
asks the user to confirm overwriting it.
The man-pages make clear that a path should be provided. If none is 
provided, a message is issued informing the user that results are saved 
in the home directory. By using tempdir() the user would have to move 
all files to another location before the session is closed.

Helmut

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      David Cortes
    

    

      
      #
    

  


  

      
It is explained here:
https://cran.r-project.org/web/packages/policies.html
Section about source packages:
"Packages should not write in the user?s home filespace (including
clipboards), nor anywhere else on the file system apart from the R
session?s temporary directory (or during installation in the location
pointed to by TMPDIR: and such usage should be cleaned up)."

And I think the policy was different at some point in the past. Anyway,
it's a good policy - just get a temporary folder and your desired file
name - e.g.:
file.path(tempdir(), "your_file.png")

Best regards,
David Cortes

        
On Thu, 2020-07-23 at 12:44 +0200, Helmut Sch?tz wrote:

            

      
      
      
    

  
  


  


      

    

    

      
      

        


  

        

      Helmut Schütz
    

    

      
      #
    

  


  

      
Hi David,

David Cortes wrote on 2020-07-23 13:16:

            

      
      
      
    

        
THX; I missed that! However, the policy continues:
"Limited exceptions may be allowed in interactive sessions if the 
package obtains confirmation from the user."
IMHO, this is applicable here (i.e., the user _should_ specify a 
directory (as recommended in the man-pages), and only if none is 
provided, a warning is issued and confirmation obtained).
If I would use tempdir() and the user forgets to copy the result files 
to another location and closes the console, everything would be lost and 
the user would have to start again from scratch. I think that this is 
not very user-friendly.

Helmut

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      Duncan Murdoch
    

    

      
      #
    

  


  

      
On 23/07/2020 8:18 a.m., Helmut Sch?tz wrote:

            

      
      
      
    

        
I would issue an error instead of a warning, and in the error message, 
suggest some code that should work.

Duncan Murdoch

  
  


  


      

    

    

      
      

        


  

        

      Dirk Eddelbuettel
    

    

      
      #
    

  


  

      
Helmut,

For previous uploads you affirmed that you read the CRAN Repository Policy
which states

     * The code and examples provided in a package should never do anything
       which might be regarded as malicious or anti-social. The following are
       illustrative examples from past experience.

       [...]

          * - Packages should not write in the user?s home filespace
            (including clipboards), nor anywhere else on the file system
            apart from the R session?s temporary directory (or during
            installation in the location pointed to by TMPDIR: and such usage
            should be cleaned up). Installing into the system?s R
            installation (e.g., scripts to its bin directory) is not allowed.

Your package appears to violate that requirement. I would fix the package.

Dirk

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      Sebastian Meyer
    

    

      
      #
    

  


  

      
Back to the original topic: graphics.off() is probably not what you
want. It shuts down *all* open graphics devices, not just the current
one. Example code or your plotting functions should not do that.

Calling graphics.off() in example code will also disturb standard R CMD
check. Before running the examples, R CMD check opens a pdf device to
store any graphics output [1]. You will find the resulting pdf file in
{PACKAGE}.Rcheck/{PACKAGE}-Ex.pdf after R CMD check. [BTW, the plots
therein will be usefully annotated with the names of the originating
help pages.]

R CMD check will eventually fail from trying to close this pdf device
after running the examples [2], if you have already closed all graphics
devices (including this pdf device) through code in your examples. This
is where the

Error in grDevices::dev.off() :
  cannot shut down device 1 (the null device)
Execution halted

actually came from.

Finally, many of the graphics devices are platform-specific and the png
device may not even be available. So it is reasonable to condition on
capabilities("png") or to put such examples in \donttest. The latter is
also used in the example code for grDevices::png, at least in the Unix
version of the man page [3].

HTH!

	Sebastian


[1]: file.path(R.home("share"), "R", "examples-header.R")
[2]: file.path(R.home("share"), "R", "examples-footer.R")
[3]:
https://github.com/r-devel/r-svn/blob/15253534aa1f4e91d33d9b0e3f035daedfe750bb/src/library/grDevices/man/unix/png.Rd#L249-L259

BTW, on Unix-alikes, example(png) writes to files myplot.png,
myplot1.jpeg, and myplot2.jpeg in the current working directory. This
should be fixed.



Am 22.07.20 um 19:25 schrieb Helmut Sch?tz:

            

      
      
      
    

  
  


  


      

    

    

      
      

        


  

        

      Helmut Schütz
    

    

      
      #
    

  


  

      
Hi Sebastian,

Sebastian Meyer wrote on 2020-07-23 16:52:

            

      
      
      
    

        
THX!

            

      
      
      
    

        
Ah! For years I was wondering where the PDF comes from.

            

      
      
      
    

        
OK, now I have:
if (file.exists("myfile") & !is.null(dev.list()["png"]) {
 ? invisible(dev.off(dev.list()["png"]))
}
You made my day. No error in check() any more.

            

      
      
      
    

        
Ouch!

            

      
      
      
    

        
Done.

            

      
      
      
    

        
It was already in \donttest{} but check() ignored that.

            

      
      
      
    

        
It did. Additionally I've learned a new abbreviation...

Helmut

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      Helmut Schütz
    

    

      
      #
    

  


  

      
Hi Dirk,

Dirk Eddelbuettel wrote on 2020-07-23 15:16:

            

      
      
      
    

        
As I wrote previously the statement continues with
"Limited exceptions may be allowed in interactive sessions if the 
package obtains confirmation from the user."

I'm not a native speaker of English but for me "should not write" != 
"must not write".

            

      
      
      
    

        
I removed the automatic switch to "~/" if no path is given. As before I 
recommend in the man-pages to give the full path. However, I _still_ 
state that "~/" _can_ be used for convenience.
The package is used in a regulated environment. The output file contains 
an entire audit-trail (name of the user and system, version and date of 
the OS, R, all packages, functions used, time of execution, blablah). If 
the package would write to tempdir() I would have to add a warning in 
bold font of every man-page like "Execute the command tempdir() to find 
out where your result files reside. Copy the files to a safe location 
before you quit the R-session. If you fail to do so, your results will 
be lost."

Off topic: I don't like it that on Windows tempdir is located at 
"C:/Users/{Username}/AppData/Local/Temp/Rtmp..." I strictly separate my 
OSes (on C), software (on D), data (on E), backups (on F). Writing to 
the system disk is not what I prefer. However, in my installation "~/" 
resolves to "E:/Users/{Username}/Documents/"...

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      William Dunlap
    

    

      
      #
    

  


  

      
R-4.0 introduced a new function, tools::R_user_dir(package, which),
where which is one of "data", "config", and "cache".  It gives
standard directory names in which to place package-and-user-specific
files which you want to last longer than one R session.

I suppose you will still have to ask the user if it is ok to use those
directories, but you might want to put your log files in one of them
instead of in ~/ itself.

Bill Dunlap
TIBCO Software
wdunlap tibco.com

        
On Thu, Jul 23, 2020 at 2:15 PM Helmut Sch?tz <helmut.schuetz at bebac.at> wrote:

            

      
      
      
    

        

      
      
    

  
  


  


      

    

    

      
      

        


  

        

      Duncan Murdoch
    

    

      
      #
    

  


  

      
On 23/07/2020 5:11 p.m., Helmut Sch?tz wrote:

            

      
      
      
    

        
And "may be allowed" is not "will be allowed".

            

      
      
      
    

        
This is a little unclear (perhaps the language issue again).  It's fine 
if your documentation recommends the user choose that.  That's a 
variation on what I recommended to you (that you generate an error 
message that suggests how to avoid the error).

I don't agree with it if you mean to say the CRAN policy gets it wrong, 
and you should be allowed to have your original automatic fallback.

            

      
      
      
    

        
It can resolve anywhere you like (as long as its writable), by 
specifying the TMPDIR environment variable.

Duncan Murdoch

  
  


  


      

    

    

      
      

        


  

        

      Helmut Schütz
    

    

      
      #
    

  


  

      
Duncan Murdoch wrote on 2020-07-24 01:05:

            

      
      
      
    

        
Which leaves the question open _who_ may -- or may not -- allow it. ;-)

            

      
      
      
    

        
Yes, I've done that.

            

      
      
      
    

        
Agree. Though I can change it permanently in _my_ Rcmd_environ file, in 
a package I _must not_ change the users environment.

Helmut

  
    

      
      
    

  


  


      

    

    
2 days later

    

      
      

        


  

        

      Berry Boessenkool
    

    

      
      #
    

  


  

      
One (late) additional command:

in the following setup:

pdf("some_path")
plot(1)
dev.off()

if the plotting function fails, code execution stops and dev.off never gets called, leaving (potentially nested) devices open.

I suggest:

pdf("some_path")
on.exit(dev.off(), add=TRUE)
plot(1)


add=TRUE is technically only needed if you have several on.exit calls.
I trained myself to always include it after it bit me when I introduced more on.exit calls later on.

HTH,
Berry