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

Add shim for system.file to work with devtools-loaded packages #129

Merged
merged 2 commits into from
Jun 11, 2019

Conversation

wch
Copy link
Collaborator

@wch wch commented Jun 4, 2019

This addresses rstudio/shiny#2468. Now when devtools::test(, "bootstrap") is called for shiny, it no longer has errors.

@cpsievert
Copy link
Collaborator

cpsievert commented Jun 6, 2019

Hmm, I don't see any errors when I run devtools::test(, "bootstrap") on rstudio/shiny@d35c5c8

(Update: It wasn't failing because of the workaround I have in my .Rprofile)

Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

Just cause I'm curious, how does find.package() know to return the source path and not the installed path?

pkg_path <- find.package(package)

Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

Maybe we should use pkgload::dev_meta() if pkgload is installed (to help protect against internal changes to dev_meta())

if (is.null(ns) || is.null(ns$.__DEVTOOLS__)) {

@wch
Copy link
Collaborator Author

wch commented Jun 10, 2019

The reason to not use pkgload::dev_meta() is because then htmltools will take a dependency on pkgload.

@wch
Copy link
Collaborator Author

wch commented Jun 10, 2019

Just cause I'm curious, how does find.package() know to return the source path and not the installed path?

R doesn't have a concept of source path vs. installed path; it just has a concept of installed path. So when devtools loads a package, it tells R that the installed path is the source path.

@cpsievert
Copy link
Collaborator

cpsievert commented Jun 10, 2019

I was thinking we'd have pkgload in suggests (or not at all with a getFromNamespace() workaround), but I don't feel that strongly about it, so you have my 👍 either way

if (system.file(package = "pkgload") != "") {
  return(!is.null(pkgload::dev_meta(package)))
} else if (is.null(ns) || is.null(ns$.__DEVTOOLS__)) {
  return(FALSE)
}

@wch
Copy link
Collaborator Author

wch commented Jun 11, 2019

Thanks - I'd still rather not take any sort of dependency on pkgload. The worst that can happen if pkgload decides to change the .__DEVTOOLS__ thing to something else is that this will no longer work, and the user will be in the same situation that exists currently.

Also, if we do use pkgload::dev_meta, then, if the user has pkgload installed, that line will cause the pkgload package to load (along with its dependencies) just to make this check, and that's way more heavyweight than is necessary here.

@wch wch merged commit eea9211 into rstudio:master Jun 11, 2019
@wch wch deleted the shim-system-file branch June 11, 2019 02:39
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.

3 participants