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

install_svn optionally accepts revision #739

Merged
merged 16 commits into from
Apr 28, 2015
Merged

install_svn optionally accepts revision #739

merged 16 commits into from
Apr 28, 2015

Conversation

lev-kuznetsov
Copy link
Contributor

Will update to revision specified or use latest if omitted.

Also changed the way subdir parameter works for install_svn, subdir will be added to the checkout url prior to the checkout (in particular allows use of the bioconductor source repo, they close down the root, only opening up the packages, need to supply auth via args parameter)

Also fixed travis builds

@lev-kuznetsov
Copy link
Contributor Author

I don't want to seem impatient, I'm just wondering if this or something like it would be accepted since what I need to do next would be contingent on it eventually making its way upstream (or not)

@wch
Copy link
Member

wch commented Mar 18, 2015

I think this would be a useful feature to include. Why did you rename the arg from subdir to svn_subdir ? (Also I think the documentation wasn't updated for that change.)

@lev-kuznetsov
Copy link
Contributor Author

I didn't change API for install_svn, that still takes an argument named subdir, that change is only underneath for the svn_remote object.

Before my changes the subdir argument is dealt with after the checkout from root by the generic install_remote. So it checks out the entire repo and then installs only from the subfolder which would be prohibitive for large repos or impossible for repos where you cannot checkout from root due to permissions.

With this change the subdir is appended to the url prior to checkout and as far as install_remote is concerned it's always installing from root of the checkout. As far as I've used svn I've always been able to check out directly from the subfolder.

@lev-kuznetsov
Copy link
Contributor Author

Come to think of it with the revision parameter before the variadic it's conceivable that was a backward incompatible API change for install_svn, I moved is after

@wch
Copy link
Member

wch commented Mar 18, 2015

Oh, OK, that makes sense. I think that even though the change might be backward incompatible, it's a rare enough case that you don't need to worry about.

@@ -9,6 +9,8 @@
* `NOT_CRAN` is no longer set automatically if it has been set externally to
allow overriding.

* `install_svn` now optionally accepts revision number
Copy link
Member

Choose a reason for hiding this comment

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

Can you please include the issue/PR number, and thank your github user name?

@lev-kuznetsov
Copy link
Contributor Author

That's it I think

@@ -34,6 +34,15 @@ archived source tarballs and tries to install an older version instead.}
but not installed on other platforms.

The default is the appropriate binary type on Windows and on the
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you have a merge conflict here - re-documenting should fix it

@@ -64,6 +70,16 @@ remote_download.svn_remote <- function(x, quiet = FALSE) {
stop("There seems to be a problem retrieving this SVN-URL.", call. = FALSE)
}

if (!is.null(x$revision)) {
pwd <- getwd()
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, a simpler pattern is pwd <- setwd(bundle) - but I'll make this change locally.

hadley added a commit that referenced this pull request Apr 28, 2015
install_svn optionally accepts revision
@hadley hadley merged commit 906f711 into r-lib:master Apr 28, 2015
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