refactor - use pak for downloading and installing
#7
Conversation
|
I have read the CLA Document and I hereby sign the CLA |
llrs-roche
left a comment
llrs-roche
left a comment
There was a problem hiding this comment.
My review is mostly from reading the code. Are there any conditions you wanted to check it works better now? (I see lots of commits with "test" as the message)
It seems like the script itself starts on line 224, but it is not documented with a comment. It will be probably easier for future readers.
I tested locally on the teal repository and it didn't finish (but command did finish from an interactively R session):
cd ../teal
Rscript.exe ../r-revdepcheck-action/script.R ../r-revdepcheck-action/repo 2 1200
Installing required packages...
✔ Loading metadata database ... done
ℹ No downloads are needed
✔ 7 pkgs + 75 deps: kept 81 [10.7s]
── Configuration ─────────────────────────────────────────────
Missing `.revdeprefs.yaml` file.fig file...
This indicates all reverse dependencies from CRAN.
✔ Reading `.revdeprefs.yaml` config file... [368ms]
References used:
── Initiate pre-requisites ───────────────────────────────────
[1] "C:/Users/sanchosl/AppData/Local/Temp/Rtmpu2k72q/file581836218a3/src/contrib/rlang_1.1.6.tar.gz"
✔ Initiating `miniCRAN`... [4s]
Error in read.dcf("DESCRIPTION") : cannot open the connection
In addition: Warning message:
In read.dcf("DESCRIPTION") :
cannot open compressed file 'DESCRIPTION', probable reason 'No such file or directory'
Execution halted
| get_tar_gz_from_file <- function(pkg, version, path) { | ||
| path | ||
| } | ||
| get_tar_gz_from_fulltarget <- function(pkg, version, path) { | ||
| get_tar_gz_from_file(pkg, version, path) | ||
| } | ||
| get_tar_gz_from_fulltarget_tree <- function(pkg, version, path) { |
There was a problem hiding this comment.
These three functions could be reduced to one using missing() to control the behavior
| temp_dir <- tempfile() | ||
| dir.create(temp_dir) |
There was a problem hiding this comment.
temp_dir is a file. Given that pkbuild::build says that dest_path: " If it is an existing file, then it will be overwritten." It could create a temporary file on the beginning of the function and reuse that instead of creating one for each branch.
| tempdir <- tempfile() | ||
| dir.create(tempdir) | ||
| on.exit(unlink(tempdir, recursive = TRUE), add = TRUE) | ||
|
|
||
| new_file_name <- sprintf("%s_%s.tar.gz", pkg, version) | ||
| file.copy(tar_gz_path, file.path(tempdir, new_file_name)) |
There was a problem hiding this comment.
Wouldn't it be easier to use dirname(tar_gz_path) instead of copying the file to a temporary folder for miniCRAN to copy to the repository folder?
miniCRAN::addLocalPackage(pkg, dirnames(tar_gz_path), minicran_path)
There was a problem hiding this comment.
Yes it would and I really wanted to make it like this but miniCRAN::addLocalPackage complained if the parent dir contains more than the binary file for a given package which is very typical for things like cache dir - it does contain multiple files.
Just because of that - I created this helper add_to_minicran function to: (i) create empty temp dir, (ii) move from cache into that dir and finally (iii) add to miniCRAN. This function is meant exactly to address that comment
There was a problem hiding this comment.
In that case, please document a bit the function as I missed it all the reasons. (sorry for getting back to you after so late)
| } | ||
|
|
||
| args <- commandArgs(trailingOnly = TRUE) | ||
| setwd(normalizePath(file.path(args[1]))) |
There was a problem hiding this comment.
Maybe document here too that the first arguments is the repository path. Also it is not checked that the directory exists.
pakfor downloading and installingpakgives ambiguous download results - sometimes it gives.tar.gz, sometimes compressed files etc. A lot of code in this PR is to handle this.