ycphs / openxlsx

openxlsx - a fast way to read and write complex xslx files
https://ycphs.github.io/openxlsx/
Other
224 stars 75 forks source link

Check tmpDir with dir.exists() and create tmpDir non-recursively #364

Closed christianrickert closed 2 years ago

christianrickert commented 2 years ago

The creation of tmpDir fails, when the Windows system variables TEMP/TMP are set to the root directory of an (external) drive:

EnvironmentalVariables_Windows10

Warning in dir.create(path = tmpDir, recursive = TRUE) :
  cannot create dir 'T:\', reason 'Permission denied'

tempDir() and file.path(tempDir()) return the same temporary path, which leads to the described error:

> tempdir()
[1] "T:\\\\Rtmp00GJkQ"
> file.path(tempdir())
[1] "T:\\\\Rtmp00GJkQ"

When dir.create tries to recursively create the directory structure, it fails because it cannot access the mounting point of the drive ("drive letter with separator") for writing - while any files and folders placed at the root directory level are fully accessible with the respective file system permissions granted:

> tmpDir <- tempfile(pattern = "workbookTemp_", tmpdir = "T:\\")
> 
> dir.create(path = tmpDir, recursive = FALSE) # works
>
> dir.create(path = tmpDir, recursive = TRUE)  # fails
Warning message:
In dir.create(path = tmpDir, recursive = TRUE) :
  cannot create dir 'T:\', reason 'Permission denied'
>

Interestingly, the recursive creation of the tmpDir works when the tmpdir variable is set to T: instead of T:\.

> tmpDir <- tempfile(pattern = "workbookTemp_", tmpdir = "T:")
> dir.create(path = tmpDir, recursive = TRUE)
> 

There are two possible workarounds without altering the code:

  1. Parsing the paths of TEMP/TMP before passing to tempfile, and
  2. moving the TEMP/TMP paths to subdirectories of the (external) drive.

However, both options mask another problem with the current code:

In the saveWorkBook function of WorkbookClass.R, we are creating a single folder in the system's temp directory: There is no need to recursively create the directory structure for a single folder.

The system's temp folder is automatically created by tempfile(pattern = "file", tmpdir = tempdir()), see documentation:

By default, tmpdir will be the directory given by tempdir(). This will be a subdirectory of the per-session temporary directory found by the following rule when the R session is started. The environment variables TMPDIR, TMP and TEMP are checked in turn and the first found which points to a writable directory is used: if none succeeds /tmp is used. The path should not contain spaces. if none succeeds the value of R_USER (see Rconsole) is used. If the path to the directory contains a space in any of the components, the path returned will use the shortnames version of the path. Note that setting any of these environment variables in the R session has no effect on tempdir(): the per-session temporary directory is created before the interpreter is started.

The second change to the code is addressing an issue with file.exists (see: docs) and directories:

Note that the existence of a file does not imply that it is readable: for that use file.access.) What constitutes a ‘file’ is system-dependent, but should include directories. (However, directory names must not include a trailing backslash or slash on Windows.)

A better function to check if a directory exists would be dir.exists (see ?dir.exists:

dir.exists checks that the paths exist (in the same sense as file.exists) and are directories.

Please consider these code changes for review.

JanMarvin commented 2 years ago

Hi @christianrickert , thanks for the pull request, looks reasonable.

codecov-commenter commented 2 years ago

Codecov Report

Merging #364 (c9992b5) into master (2ace59a) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #364   +/-   ##
=======================================
  Coverage   67.76%   67.76%           
=======================================
  Files          34       34           
  Lines        8931     8931           
=======================================
  Hits         6052     6052           
  Misses       2879     2879           
Impacted Files Coverage Δ
R/WorkbookClass.R 60.44% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3ceeb84...c9992b5. Read the comment docs.