Merck / pkglite

Compact Package Representations
https://merck.github.io/pkglite/
GNU General Public License v3.0
30 stars 4 forks source link

Test whether output path is writable #22

Closed nanxstats closed 2 years ago

nanxstats commented 2 years ago

We should test whether the output path is writable, and give friendlier error messages if not before writing to it.

nanxstats commented 2 years ago

@elong0527 I want to hear your input on this.

I've had some trouble implementing this. There is also evidence that this shouldn't be done at all, for a good reason. ( 1, 2 )

One issue I see is that there are some conditions that could give false positives while actually writable. For example, folder2 and folder3 don't exist in /folder1/folder2/folder3/file.txt but folder1 is writable. This means that there are no trivial ways to test, unless we recursively test if the parent directories exist and writable.

To take a step back, we can scope it down to testing if "parent folder exists AND writable". dirname() can often give the correct parent directory path but is not very reliable for network drive paths. For example, dirname("\\\\something\\") will return ".".

We can, though, as suggested by the links above, use a try-catch structure and handle the error where we create a file or directory, which seems a bit excessive.

Below is what the current error messages look like.

pack() says:

Error in file(con, "w") : cannot open the connection
In addition: Warning message:
In file(con, "w") :

 Error in file(con, "w") : cannot open the connection

unpack() says:

When the parent directory exists but not writable:

Error in file(con, "w") : cannot open the connection
In addition: Warning messages:
1: In dir.create(path_dir, recursive = TRUE) :
  cannot create dir '/path/not/writable', reason 'Permission denied'
2: In file(con, "w") :

 Error in file(con, "w") : cannot open the connection 

When parent directory does not exist but the entire path is writable:

Warning messages:
1: In dir.create(output) :
  cannot create dir '/path/not/exist/', reason 'No such file or directory'
2: In normalizePath(path.expand(path), winslash, mustWork) :
  path[1]="/path/not/exist/": The system cannot find the path specified

rmarkdown::render() didn't handle this specifically:

Quitting from lines 25-26 (zzz.Rmd) 
Error in png(..., res = dpi, units = "in") : unable to start png() device
In addition: Warning messages:
1: In dir.create(dirname(name), recursive = TRUE) :
  cannot create dir '/path/not/writable/zzz_files', reason 'Permission denied'
2: In png(..., res = dpi, units = "in") :
  unable to open file '/path/not/writable/zzz_files/figure-html/pressure-1.png' for writing
3: In png(..., res = dpi, units = "in") : opening device failed

readr::write_lines() is a bit more concise (thus more informative?):

Error: Cannot open file for writing:
* '/path/not/exist/or/writable.txt'
elong0527 commented 2 years ago

I support to skip this item :)

For the issue of parent folder, would you feel we can test if the exact path exists first? https://fs.r-lib.org/reference/file_access.html

The logic of writable only execute if the full path exists.

nanxstats commented 2 years ago

Good, then let's skip it for now. 👍

Yes, one can use base::dir.exists() to test if the parent folder exists, or using its fs equivalent.

The main issues is actually before that: I cannot find a reliable way to return the parent directory path: