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

Accommodating custom GitHub URL's #506

Closed
wants to merge 24 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ghuiber

ghuiber commented Jun 24, 2014

devtools::install_github() might accept an extra parameter, repo_path, that defaults to https://github.com/ but can point to a custom Enterprise GitHub URL.

This default ensures that this feature will change nothing in the way this function is used now, but the new parameter would allow devtools to be used as a way to disseminate packages between R users who are members of closed Enterprise GitHub communities.

ghuiber added some commits Jun 24, 2014

Added param for alternate github repo path
Hoping to get install_github() to work with an enterprise GitHub repo
with a different path than https://github.com
@hadley

This comment has been minimized.

Member

hadley commented Jun 25, 2014

Thanks! Two comments:

  1. I'm not sure repo_path is the right name for that parameter. Maybe github_url?
  2. Maybe it should be a separate function, install_github_enterprise(), which could then pull the github url from an environment variable or R option so you didn't have to set it every time?

cc @cscheid, @hilaryparker

@hilaryparker

This comment has been minimized.

hilaryparker commented Jun 25, 2014

Nice!!

I agree that github_url is probably more intuitive than repo_path.

I'm pretty agnostic to it being a separate function vs. an option in install_github(). For my use case it's mostly me just installing my own functions, so I'm not often calling the function to begin with.

@hadley

This comment has been minimized.

Member

hadley commented Jun 25, 2014

@hilaryparker btw the way @ghuiber was inspired to start writing packages (and hence this pull request) because of your packages blog post 😄

@hilaryparker

This comment has been minimized.

hilaryparker commented Jun 25, 2014

YAY!! It comes full circle.. now he's helping me! :D

@tbates

This comment has been minimized.

tbates commented Jun 25, 2014

install_github_enterprise() would be nice with an accompanying
devtools_git_enterprise() function which returned this info when called with no arguments and wrote them to the environment when given a parameter value.

then this would work:

install_github_enterprise(repo="kickass", github_url = devtools_git_enterprise() ) 
@ghuiber

This comment has been minimized.

ghuiber commented Jun 25, 2014

I like github_url much better than repo_path, so I made the change, recompiled the docs, tested, built, installed, committed and pushed. That was an easy fix.

As to install_github() vs. install_github_enterprise(): what got me to nose around devtools in the first place was my client's wondering whether there was such a thing as install_github_[your company name here](). That's one piece of evidence that people would indeed find a separate function a more convenient place for a custom URL functionality than another argument to install_github(). The latter was just easier for me to implement.

How would I go about writing it with as little code duplication as possible? Add an install_github_enterprise() function definition to my install_github.r as a kind of convenience wrapper? This way my current version of install_github() continues to work with github_url explicitly set, or people can ignore that part altogether and just use install_github_enterprise().

Finally, I have no idea about two unrelated bits

  1. How to do what @tbates suggests
  2. What to make of this Travis CI build fail. I'd like to make this pull request as easy on @hadley as I can, but I'm at a loss. Some shell script seems to be the snag, but what do I have to do with it?
@@ -0,0 +1,6 @@
#include <R.h>

This comment has been minimized.

@hadley

hadley Jun 25, 2014

Member

Can you please remove these two files?

This comment has been minimized.

@ghuiber

ghuiber Jun 25, 2014

Deleted src-i386/devtools.c and src-x64/devtools.c.

On Wed, Jun 25, 2014 at 10:04 AM, Hadley Wickham notifications@github.com
wrote:

In src-i386/devtools.c:

@@ -0,0 +1,6 @@
+#include <R.h>

Can you please remove these two files?


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

@hadley

This comment has been minimized.

Member

hadley commented Jun 25, 2014

I'd make install_github_enterprise() take a github_url argument, and then have install_github() call it with https://github.com. To implement Tim's idea, you'd write a function that guessed the enterprise url (probably from Sys.getenv(), or getOption() or both) and then make that the default argument to github_url.

@hadley

This comment has been minimized.

Member

hadley commented Jun 25, 2014

The build looks like it's failing for other reasons - I think because I haven't updated the docs for some previous changes that I made.

@ghuiber

This comment has been minimized.

ghuiber commented Jun 26, 2014

Done. There is now an install_github_enterprise() wrapper for install_github(), and a helper devtools_git_enterprise() that retrieves the GITHUB_URL environment variable as Tim suggested.

@ghuiber

This comment has been minimized.

ghuiber commented Jun 27, 2014

Resolved merge conflict in my install-github.r to accommodate new GitHub API call #474. Travis CI build probably fails for whatever the original reason was 3 days ago.

@hadley

This comment has been minimized.

Member

hadley commented Jun 27, 2014

Now it fails for a legitimate reason - check https://travis-ci.org/hadley/devtools/builds/28599041#L963

@ghuiber

This comment has been minimized.

ghuiber commented Jun 27, 2014

Well, one warning still squeaked by, but I don't think it's aimed at me. It has to do with duplicate references to this SVN functionality. See https://travis-ci.org/hadley/devtools/builds/28632916#L954

@ghuiber

This comment has been minimized.

ghuiber commented Jul 1, 2014

I have no idea what's going on here. Travis turns up the same lone warning about one Rd file with duplicated names: svn_path.Rd. I have seen something to do with this in my test() output, shown below, but I also saw some other warnings which I thought would be both more serious and my own fault.

I would appreciate any hints:

<snip>
Warning messages:
1: install_svn.Rd not generated by roxygen2. Skipped. 
2: install_svn_single.Rd not generated by roxygen2. Skipped. 
Testing devtools
Loading devtools
Checks : .
Data : ............................
Dependencies : ..............
DESCRIPTION checks : ..
Compiled DLLs : ................
GitHub : ........................
help : ...................
Imports : .....
Load: collate : ......
Load hooks : ....................................
Metadata : .................
Namespace : .............................
Rtools tests : ....
s4-export : .
s4-unload : ......................
shim : ................
Vignettes : ......................
...
With : ...............

Warning messages:
1: In if (!file.exists(pkg$path, "src")) return(TRUE) :
  the condition has length > 1 and only the first element will be used
2: In if (!file.exists(pkg$path, "src")) return(TRUE) :
  the condition has length > 1 and only the first element will be used
3: In if (!file.exists(pkg$path, "src")) return(TRUE) :
  the condition has length > 1 and only the first element will be used
4: In if (!file.exists(pkg$path, "src")) return(TRUE) :
  the condition has length > 1 and only the first element will be used
5: In if (!file.exists(pkg$path, "src")) return(TRUE) :
  the condition has length > 1 and only the first element will be used
"C:/PROGRA~1/R/bin/x64/R" --vanilla CMD build "C:\Users\ghuiber\Documents\GitHub\devtools" --no-manual  \
  --no-resave-data 

* checking for file 'C:\Users\ghuiber\Documents\GitHub\devtools/DESCRIPTION' ... OK
* preparing 'devtools':
* checking DESCRIPTION meta-information ... OK
* cleaning src
* checking for LF line-endings in source and make files
* checking for empty or unneeded directories
Removed empty directory 'devtools/tests/testthat/testMarkdownVignettes/inst/doc'
Removed empty directory 'devtools/tests/testthat/testMarkdownVignettes/inst'
Removed empty directory 'devtools/tests/testthat/testVignetteExtras/inst/doc'
Removed empty directory 'devtools/tests/testthat/testVignetteExtras/inst'
Removed empty directory 'devtools/tests/testthat/testVignettes/inst/doc'
Removed empty directory 'devtools/tests/testthat/testVignettes/inst'
* building 'devtools_1.5.0.99.tar.gz'

[1] "C:/Users/ghuiber/Documents/GitHub/devtools_1.5.0.99.tar.gz"
Warning message:
In if (!file.exists(pkg$path, "src")) return(TRUE) :
  the condition has length > 1 and only the first element will be used

This is all. Travis is not worried about any of the pkg$path condition length warnings. My local version of install_github_enterprise() works.

@ghuiber

This comment has been minimized.

ghuiber commented Aug 15, 2014

All may look well, but it's not. While load_all(); document(); test(); build() went without a hitch and Travis CI build passed too, now public GitHub doesn't work either. My API v3 call to get the archive zip file goes through, which is progress, but upon attempting actual installation I get this:

> library(devtools)
> install_github('ghuiber/syncR',auth_token=Sys.getenv()[['GITHUB_PAT_PUBLIC']])
Installing github repo ghuiber/syncR@master from ghuiber
Downloading syncR.zip from https://api.github.com/repos/ghuiber/syncR/zipball/master
Installing package from C:\Users\ghuiber\AppData\Local\Temp\Rtmp6Rb2qt/syncR.zip
Error in system(full, intern = quiet, ignore.stderr = quiet, ...) : 
  '"internal"' not found

Any ideas? Maybe Rtools rotted again? I re-installed them not a month ago.

@ghuiber

This comment has been minimized.

ghuiber commented Aug 15, 2014

All seems well, but it's not and re-installing Rtools did not fix the problem. This version of devtools will choke on known, working GitHub packages such as Slidify with the same error message:

> install_github("ramnathv/slidify")
Using github PAT from envvar GITHUB_PAT
Installing github repo ramnathv/slidify@master from ramnathv
Downloading slidify.zip from https://api.github.com/repos/ramnathv/slidify/zipball/master
Installing package from C:\Users\ghuiber\AppData\Local\Temp\RtmpQN2X5y/slidify.zip
Error in system(full, intern = quiet, ignore.stderr = quiet, ...) : 
  '"internal"' not found
> install_github("ramnathv/slidify",auth_token=NULL)
Installing github repo ramnathv/slidify@master from ramnathv
Downloading slidify.zip from https://api.github.com/repos/ramnathv/slidify/zipball/master
Installing package from C:\Users\ghuiber\AppData\Local\Temp\RtmpQN2X5y/slidify.zip
Error in system(full, intern = quiet, ignore.stderr = quiet, ...) : 
  '"internal"' not found
> 

Edit: this works on the Mac though (OS X Mavericks, R 3.1.1).

Fixed url param in github_ref.github_pull()
The list element param$github_url does not exist so it’s always NULL.
The function should check instead the element param$url, which may or
may not be NULL.
@hadley

This comment has been minimized.

Member

hadley commented Aug 20, 2014

Looks like I broke something in decompress. Will take a look tomorrow when I'm back at a computer

@hadley hadley closed this in 41a57a4 Aug 21, 2014

@hadley

This comment has been minimized.

Member

hadley commented Aug 21, 2014

Now that the install_github() refactoring is complete, this is really easy for me to implement. I also think now that it doesn't need it's own function.

It's not quite as nice as your implementation (since you have always have to specify host), but that does have the advantage of being more explicit/reproducible.

@ghuiber

This comment has been minimized.

ghuiber commented Aug 21, 2014

Thank you so much! I can't wait for the next CRAN release :)

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