Update how `tar` binaries are discovered

toph-allen commented 2 years ago

This PR incorporates #678, but goes further, so will supersede that PR.

Fixes #648

Intent

Packrat has previously forced the use of R's internal tar implementation. This is buggy in a number of situations, including any repository with a slug > 16 characters.

Note: The code is ready for review, but QA notes are incomplete.

Approach

Influenced by the approach taken in renv, we now define a function tar_binary() to encapsulate slightly more complex logic:

If TAR is set, respect that;
If on Unix, look for tar on the PATH;
If on Windows, look for the system tar and use that if it exists (see renv implementation)
Otherwise, return "internal" and maybe emit a warning about long filenames.

I put the function in env.R, because it gathers information about the environment.

There were a number of locations where tar() or untar() were called with internal, and this PR changes all of them.

Automated Tests

This PR adds automated tests. Note that it adds a dependency on mockery for the tests; I'm not sure if we're ok with this.

There are five test functions that test the logical flow of the function in the following cases:

A TAR variable is defined returns the contents of that variable
We're on Unix and we can find a tar binary on the PATH returns that binary
We're on Unix and we can't find a tar binary on the PATH returns "internal"
We're on Windows and we find system tar returns the system tar
We're on Windows and we can't find system tar returns "internal"

QA Notes

You can validate that packrat::restore() still works by installing the the version of the package from this branch and running packrat::restore() in an R session in the top level directory extracted from this .zip file: simple-rmd.zip

This will perform a Packrat restore. I'm not sure if there's a way to validate that the download path has been exercised from restore() though.

You can also run packrat:::tar_binary() and see that it prints the expected path for your environment. For me, this looks like:

> packrat:::tar_binary()
[1] "/usr/bin/tar"

ChaitaC commented 2 years ago

Packrat


RemoteRef: toph-untar-handling
RemoteSha: 108183392a8d733ed6bf66da65afab5b6de1c11c
GithubRepo: packrat
GithubUsername: rstudio
GithubRef: toph-untar-handling
GithubSHA1: 108183392a8d733ed6bf66da65afab5b6de1c11c
NeedsCompilation: no
Packaged: 2022-08-09 18:31:24 UTC; chaitamacpro
Built: R 4.1.1; ; 2022-08-09 18:31:25 UTC; unix

ChaitaC commented 2 years ago

verified on packrat branchtoph-untar-handling

rstudio / packrat