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

Provide option to disable internet connections #774

Merged
merged 2 commits into from Jul 25, 2018

Conversation

Projects
None yet
5 participants
@jayhesselberth
Copy link
Collaborator

jayhesselberth commented Jul 25, 2018

Closes #773. Closes #762. Closes #766.

@jayhesselberth jayhesselberth requested a review from hadley Jul 25, 2018

@hadley

hadley approved these changes Jul 25, 2018

@jayhesselberth jayhesselberth merged commit 56f3a65 into master Jul 25, 2018

@jayhesselberth jayhesselberth deleted the internet-opt branch Jul 25, 2018

@dongzhuoer

This comment has been minimized.

Copy link

dongzhuoer commented Jul 27, 2018

Thank you very much for the code, but I'm afraid that there is still one step to get it work.

I set options(pkgdown.internet = F) in my ~/.Rprofile, then

> pkgdown::build_home()
── Building home ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Reading 'news.md'
Writing 'index.html'
── Previewing site ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
> pkgdown::build_site()
══ Building pkgdown site ═══════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Reading from: '/media/computer/work/R/hgnc'
Writing to:   '/media/computer/work/R/hgnc/docs'
── Initialising site ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Writing 'sitemap.xml'Edit '_pkgdown.yml'
── Building home ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Reading 'news.md'
Error in curl::curl_fetch_memory(url, handle = handle) : 
  Could not resolve host: cloud.r-project.org
Calls: withCallingHandlers ... request_fetch.write_memory -> <Anonymous> -> .handleSimpleError -> h
Execution halted
Error in curl::curl_fetch_memory(url, handle = handle) : 
  Could not resolve host: cloud.r-project.org
@dongzhuoer

This comment has been minimized.

Copy link

dongzhuoer commented Jul 27, 2018

I found that build_site() use new_process = TRUE by default.

In this scenario, I think it's quite intuitive to enable .Rprofile in the new process or at least add option to let the user explicitly specify it.

Otherwise, user have to run pkgdown::build_site(new_process = F) (I don't think it's a good idea to have to run pkgdown in a dirty process in order to get pkgdown.internet working)

@jayhesselberth

This comment has been minimized.

Copy link
Collaborator Author

jayhesselberth commented Jul 27, 2018

I think we need to add user_profile = TRUE to the callr::r() call.

But then you have to set the option via .Rprofile.

@dongzhuoer

This comment has been minimized.

Copy link

dongzhuoer commented Jul 27, 2018

Yeah, I think it's the natural way.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jul 27, 2018

I think it would be better to set the option explicitly in the new process

@dongzhuoer

This comment has been minimized.

Copy link

dongzhuoer commented Jul 28, 2018

How can you achieve that? build_site(process_setup = function() {options(pkgdown.internet = F)})? I think the syntax is ugly.

Instead .Rprofile is the natural way to run set up code before a R process begins. we can use build_site(use_Rprofile = TRUE).

The only drawback is when user want to run some code in build_site(new_process = T), but not in build_site(new_process = F). However, in normal case, I think this need is rare.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jul 28, 2018

pkgdown would do it for you.

Writing to .Rprofile is not a good solution.

@dongzhuoer

This comment has been minimized.

Copy link

dongzhuoer commented Jul 29, 2018

If the user develops multiple packages, specify that in every build_site() is rather tedious. .Rprofile is a much better choice.

Another way is to set it in _pkgdown.yml, like what I proposed in #762 (comment)

@dongzhuoer

This comment has been minimized.

Copy link

dongzhuoer commented Jul 29, 2018

Maybe we can have something like

setup:
  code: options(pkgdown.internet = F)

or

setup:
  options:
    pkgdown.internet: F
@petermeissner

This comment has been minimized.

Copy link

petermeissner commented Jul 31, 2018

👍 For me the builds now run through with:

options(pkgdown.internet = FALSE)
pkgdown::buil_site(new_process = FALSE)

⚡️ But there are more problems ahead. Since internet is no option looking at the docs in a browser will result in several files the browser cannot obtain:

  • jquery-3.1.0.min.js
  • bootstrap.min.css
  • bootstrap.min.js
  • font-awesome.min.css
  • clipboard.min.js
  • sticky-kit.js
  • Math.Jax.js?config=Tex-AMS-MML_HTMLorMML
@dongzhuoer

This comment has been minimized.

Copy link

dongzhuoer commented Aug 1, 2018

@petermeissner I know this way, the down point is that we have to run pkgdown in a dirty session.

@dongzhuoer

This comment has been minimized.

Copy link

dongzhuoer commented Aug 1, 2018

I think we might have gone so far.

What I'm talking about originally is to disable internet for building the documentation, not for viewing the documentation.

Maybe we need separate option for these two senarios.

@BruceZhaoR

This comment has been minimized.

Copy link

BruceZhaoR commented Aug 20, 2018

@jayhesselberth 56f3a65#r30192460

https://cran.r-project.org/package= may be better than https://cloud.r-project.org/package= ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment