Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing untar on Windows #172

Merged
merged 8 commits into from Oct 2, 2018
Merged

Fixing untar on Windows #172

merged 8 commits into from Oct 2, 2018

Conversation

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Sep 27, 2018

utils::untar() calls system() with intern = TRUE,
so we need to check for attr(status, "status") as well.
In addition we silence the first try(), and give an
explicit message about retrying instead. See also #151.

Closes #171.

gaborcsardi added 2 commits Sep 27, 2018
`utils::untar()` calls `system()` with `intern = TRUE`,
so we need to check for `attr(status, "status")` as well.
In addition we silence the first `try()`, and give an
explicit message about retrying instead. See also #151.

Closes #171.
@codecov-io
Copy link

@codecov-io codecov-io commented Sep 27, 2018

Codecov Report

Merging #172 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage   91.82%   91.74%   -0.08%     
==========================================
  Files          29       29              
  Lines        1871     2047     +176     
==========================================
+ Hits         1718     1878     +160     
- Misses        153      169      +16
Impacted Files Coverage Δ
R/utils.R 88.72% <100%> (+3.25%) ⬆️
R/install.R 90.59% <0%> (-3.23%) ⬇️
R/devel.R 100% <0%> (ø) ⬆️
R/install-bitbucket.R 94.44% <0%> (+1.29%) ⬆️
R/download.R 75.34% <0%> (+1.34%) ⬆️

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 42045fc...3fc2c55. Read the comment docs.

Loading

@gaborcsardi
Copy link
Contributor Author

@gaborcsardi gaborcsardi commented Oct 2, 2018

It seems like I cannot suppress the output from untar(). I'll try a different approach and call tar --help first, and see if --force-local is mentioned there.

Loading

@gaborcsardi
Copy link
Contributor Author

@gaborcsardi gaborcsardi commented Oct 2, 2018

@jimhester I think this is good now.

Loading

R/utils.R Outdated
@@ -157,17 +157,31 @@ with_rprofile_user <- function(new, code) {

untar <- function(tarfile, ...) {
if (os_type() == "windows") {
status <- try(utils::untar(tarfile, extras = "--force-local", ...))
if(inherits(status, "try-error") || status != 0){
tarhelp <- system2("tar", "--help", stdout = TRUE, stderr = TRUE)
Copy link
Member

@jimhester jimhester Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Sys.getenv("TAR") is 'internal' we should probably just skip this completely. Also what does system2() do if there is no tar executable on the PATH?

OTOH extras is ignored if using the internal tar implementation IIRC, so maybe it is fine as is.

Loading

Copy link
Contributor Author

@gaborcsardi gaborcsardi Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, system2 needs to be tryCatch()-ed. Thanks for catching this.

Loading

Copy link
Contributor Author

@gaborcsardi gaborcsardi Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Loading

R/utils.R Outdated
silent = TRUE)
if (inherits(status, "try-error") ||
is_error_status(status) || is_error_status(attr(status, "status"))) {
message("External tar failed with `--force-local`, trying without")
utils::untar(tarfile, ...)
Copy link
Member

@jimhester jimhester Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are three cases that all end with utils::untar(tarfile, ...). So we should be able to rewrite the case that returns the status with an early return and have the rest of the cases fall through.

Loading

Copy link
Contributor Author

@gaborcsardi gaborcsardi Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but it was actually less readable than the current code. You cannot mark "not done yet" with status = NULL, because maybe that's a legit return value. Do you need a done variable, plus status ... anyway, I'll give it another try.

Loading

Copy link
Contributor Author

@gaborcsardi gaborcsardi Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better now, I think.

Loading

@gaborcsardi
Copy link
Contributor Author

@gaborcsardi gaborcsardi commented Oct 2, 2018

@jimhester done. tested on win, and CI passed as well

Loading

R/utils.R Outdated
silent = TRUE)
if (! inherits(status, "try-error") &&
! is_error_status(status) &&
!is_error_status(attr(status, "status"))) {
Copy link
Member

@jimhester jimhester Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just roll these all into is_error_status()?

Loading

Copy link
Member

@jimhester jimhester left a comment

LGTM, much easier to read now, thanks!

Loading

@gaborcsardi gaborcsardi merged commit fcd9f65 into master Oct 2, 2018
0 of 3 checks passed
Loading
@gaborcsardi
Copy link
Contributor Author

@gaborcsardi gaborcsardi commented Oct 2, 2018

Thanks!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants