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

Fix backslash issue when using tmpdir as regexp on Windows in install_theme #276

Merged
merged 1 commit into from
Mar 12, 2018

Conversation

gadenbuie
Copy link
Member

As reported at https://stackoverflow.com/questions/49212335/r-blogdown-new-site-throws-error, the use of the tempfile() result as a regexp in install_theme() causes problems on Windows.

What I found was that tmpdir = tempfile("", ".") returns a string like ".\\dir", whereas the filenames returned by utils::unzip(..., exdir = tmpdir) are "./dir/zip_base_dir.

For example:

Browse[2]> tmpdir
[1] ".\\2a0c6c64f7e"
Browse[2]> zipdir
[1] "./2a0c6c64f7e/hugo-lithium-theme-master"

This PR replaces the double backslash with a single forward slash when stripping the tmpdir prefix from zipdir and seems to work on Windows and Unix-alike (where there are no backslashes to match).

Session info for Windows test
> sessioninfo::session_info()
─ Session info ──────────────────────────────────────────────────────────────────────────────
 setting  value                       
 version  R version 3.4.0 (2017-04-21)
 os       Windows 7 x64 SP 1          
 system   x86_64, mingw32             
 ui       RStudio                     
 language (EN)                        
 collate  English_United States.1252  
 tz       America/New_York            
 date     2018-03-11                  

─ Packages ──────────────────────────────────────────────────────────────────────────────────
 package     * version date       source        
 backports     1.0.5   2017-01-18 CRAN (R 3.4.0)
 blogdown    * 0.5.9   <NA>       local         
 bookdown      0.7     2018-02-18 CRAN (R 3.4.3)
 clisymbols    1.2.0   2017-05-21 CRAN (R 3.4.3)
 commonmark    1.4     2017-09-01 CRAN (R 3.4.3)
 devtools      1.13.5  2018-02-18 CRAN (R 3.4.3)
 digest        0.6.15  2018-01-28 CRAN (R 3.4.3)
 evaluate      0.10    2016-10-11 CRAN (R 3.4.0)
 htmltools     0.3.6   2017-04-28 CRAN (R 3.4.3)
 httpuv        1.3.6.2 2018-03-02 CRAN (R 3.4.3)
 knitr         1.20    2018-02-20 CRAN (R 3.4.3)
 later         0.7.1   2018-03-07 CRAN (R 3.4.3)
 magrittr      1.5     2014-11-22 CRAN (R 3.4.0)
 memoise       1.1.0   2017-04-21 CRAN (R 3.4.0)
 mime          0.5     2016-07-07 CRAN (R 3.4.0)
 R6            2.2.2   2017-06-17 CRAN (R 3.4.3)
 Rcpp          0.12.10 2017-03-19 CRAN (R 3.4.0)
 RcppTOML      0.1.3   2017-04-25 CRAN (R 3.4.3)
 rlang         0.2.0   2018-02-20 CRAN (R 3.4.3)
 rmarkdown     1.9     2018-03-01 CRAN (R 3.4.3)
 roxygen2      6.0.1   2017-02-06 CRAN (R 3.4.3)
 rprojroot     1.2     2017-01-16 CRAN (R 3.4.0)
 rstudioapi    0.7     2017-09-07 CRAN (R 3.4.3)
 servr         0.8     2017-11-06 CRAN (R 3.4.3)
 sessioninfo   1.0.0   2017-06-21 CRAN (R 3.4.3)
 stringi       1.1.6   2017-11-17 CRAN (R 3.4.2)
 stringr       1.3.0   2018-02-19 CRAN (R 3.4.3)
 withr         2.1.1   2017-12-19 CRAN (R 3.4.3)
 xfun          0.1     2018-01-22 CRAN (R 3.4.3)
 xml2          1.2.0   2018-01-24 CRAN (R 3.4.3)
 yaml          2.1.18  2018-03-08 CRAN (R 3.4.3)

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Hmm. It is a little odd that unzip() always returns paths that use / as the path separator.

@yihui yihui added this to the v0.6 milestone Mar 12, 2018
@yihui yihui merged commit c8738e2 into rstudio:master Mar 12, 2018
@yihui
Copy link
Member

yihui commented Mar 12, 2018

Thanks! Could you also post an answer to the SO question?

BTW, in this particular case, it is absolutely appropriate for the user to report to the Github repo here (because it is a bug). In general, I hope they could try StackOverflow first:

because average users probably don't really know if an issue is a bug or misuse. I'd rather bugs being reported on StackOverflow (I have subscribed to the relevant tags by RSS) than questions being asked in a Github repo, because I cannot handle the latter.

yihui added a commit that referenced this pull request Mar 12, 2018
…#276)

change gsub() to sub() because we only want to substitute the first instance of tmpdir in zipdir; add fixed = TRUE because we only want to substitute the fixed string
@gadenbuie
Copy link
Member Author

Sure, just posted an answer. I completely understand SO vs. GH, I've been watching a couple repos and definitely see the logic to that ordering ;)

Thank you for the code review and notes in 2823c95, those changes make a lot of sense. Hope you're feeling better!

@gadenbuie gadenbuie deleted the fix-install-theme-windows branch March 16, 2018 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants