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/install GitHub submodule warn #751

Merged
merged 8 commits into from
Apr 28, 2015

Conversation

ashander
Copy link
Contributor

Add warning when downloading from github repo that uses submodules
(assessed through presence of .gitsubmodules file).

Help for install_github updated to suggest use of install_git with proper args in this case.

Limited testing (with mock responses from download and attempt to download .gitsubmodules).

Partially responsive to #737

Warn user repo contains submodules and may not behave as expeccted.

Use of submodule determined by mere presence of .gitmodules,
which is retrieved from
https://api.github.com/repos/<user>/<repo>/contents/.gitmodules?ref=<ref>
with <user>, <repo>, and <ref> determined as in install_github.
Added to @details section of helpfile along with suggested
workaround.
Requires new utilty function download_text to allow mocking
responses that should and should not raise warnings
(And suggest proper args to avoid submodule issue)
@ashander
Copy link
Contributor Author

Travis CI fails with message below. Seems like a separate issue.

Warning message:

package ‘BiocInstaller’ is not available (for R version 3.1.3)

The command "Rscript -e 'options(repos = "http://cran.rstudio.com"); tryCatch({ deps <- devtools::install_deps(dependencies = TRUE) }, error = function(e) { message(e); q(status=1) }); if (!all(deps %in% installed.packages())) { q(status = 1, save = "no") }'" failed and exited with 1 during .

@ashander
Copy link
Contributor Author

Yes looks like @jimhester has a fix in PR #746

@@ -98,6 +106,10 @@ remote_download.github_remote <- function(x, quiet = FALSE) {
} else {
auth <- NULL
}
## if repo uses submodules warn user
Copy link
Member

Choose a reason for hiding this comment

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

  1. I think the comment is extraneous (given how simple the code is)
  2. Can you please use a space after if
  3. warning() looks like it's indented too much
  4. I always use call. = FALSE with warning
  5. I think it would be easier to understand if src_submodules was defined in front of this block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Sorry for missing the indent and if-paren style with this PR. 

I've made the changes, and also fixed indent style in changes the tests

- Jaime

On Tue, Apr 21, 2015 at 6:51 AM, Hadley Wickham [email protected]
wrote:

@@ -98,6 +106,10 @@ remote_download.github_remote <- function(x, quiet = FALSE) {
} else {
auth <- NULL
}

  • if repo uses submodules warn user

  • I think the comment is extraneous (given how simple the code is)
  • Can you please use a space after if
  • warning() looks like it's indented too much

4. I always use call. = FALSE with warning

Reply to this email directly or view it on GitHub:
https://github.com/hadley/devtools/pull/751/files#r28778939

@hadley
Copy link
Member

hadley commented Apr 28, 2015

Looks good! Could you please add a bullet point to NEWS, thanking your github username?

(FYI github only notifies me about comments, not commits, so when done please add a comment like "PTAL")

@hadley hadley closed this Apr 28, 2015
@hadley hadley reopened this Apr 28, 2015
@ashander
Copy link
Contributor Author

Done! PTAL

- Jaime

On Tue, Apr 28, 2015 at 6:18 AM, Hadley Wickham [email protected]
wrote:

Looks good! Could you please add a bullet point to NEWS, thanking your github username?

(FYI github only notifies me about comments, not commits, so when done please add a comment like "PTAL")

Reply to this email directly or view it on GitHub:
#751 (comment)

@hadley hadley merged commit 07cfad5 into r-lib:master Apr 28, 2015
@hadley
Copy link
Member

hadley commented Apr 28, 2015

Thanks!

@jonkeane
Copy link

I'm attempting to install from a github repository that has a submodule. I've tried to use install_git('[url to repo]", args="--recursive") but I get a warning that args is deprecated. Is there a new prefered method for installing submodules? Now that args is deprecated with install_git()?

Sorry if reviving this issue is inappropriate, I'm happy to start a new one if that is preferred.

@ashander
Copy link
Contributor Author

I can't say for certain. But, it seems adopting git2r traded stability for the args. See 1b7e67a

Rather than commenting here, it's probably better to open a new issue regarding git2r and args (unless that discussion has already happened -- I haven't searched). In that issue you could reference this PR.

In any case, the suggestion to use install_git with --args should be removed from the docs to install-github.r

@jimhester
Copy link
Member

@jonkeane install_git() uses libgit2 rather than the git client to clone the git repository. git2r the R client to the libgit2 library does not look it has an interface for submodules on cursory inspection.

Probably the easiest thing to do assuming you do have the command line git client installed is define a simple helper function to clone the repository locally and install from there.

install_submodule_git <- function(x, ...) {
  install_dir <- tempfile()
  system(paste("git clone --recursive", shQuote(x), shQuote(install_dir)))
  devtools::install(install_dir, ...)
}
install_submodule_git("https://github.com/jonkeane/mocapGrip")
list.files(system.file(package = "mocapGrip", "python", "pyelan"))
#> [1] "__init__.py"      "elanGen.py"       "elanSkeleton.eaf" "pyelan.py"        "README.md"        "relPathFix.py"

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.

4 participants