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 sha (second attempt) #1027

Merged
merged 34 commits into from Jan 21, 2016

Conversation

Projects
None yet
3 participants
@rmflight
Contributor

rmflight commented Jan 15, 2016

@jimhester, based on our earlier conversations, here is a second attempt at adding the SHA1 to the package install. It uses git_committed as we discussed as the default function of add_sha, and then if that passes, will then use install_local to copy the package to a temp directory and do the install from there, which automatically adds the SHA1.

Note that the call to install_local probably needs to have all the other install arguments added to it to truly work as intended, but this currently passes all the tests, including the ones I added.

If this passes muster, I will add the rest of the install arguments to the install_local call.

The only ugly part of this I don't like is the return() statement that keeps a second round of install from happening and installing the one in the current directory, because that causes a NULL return value. I don't know how to avoid that ATM.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

Here's an alternative plan that might make things simpler. How about instead of having add_metadata() add the metadata to the DESCRIPTION before it gets installed (https://github.com/hadley/devtools/blob/master/R/install-remote.R#L20) we add a metadata= argument to install and have the remotes pass the metadata to it and add it after installation (e.g. at https://github.com/hadley/devtools/blob/master/R/install.r#L106).

That removes the whole chicken and the egg issue and we can just write a function for metadata= that by default checks if the git working directory is clean and adds the SHA metadata if so.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

See proof on concept jimhester@aa15cd0 which works with limited testing on my machine.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

Also note this is using remote_metadata.local_remote directly, but we will probably want to define a new function that uses your git_committed() function instead of just uses_git() in the if statement.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

OK, I like this idea, but have some questions:

  1. I didn't think what I ended up doing in this pull request was that complicated. Sure it is a 2 pass call to install, but it keeps it doing what it does best.
  2. This is completely different than the approach that all of the other install_ use to add metadata, and I worry about debugging that in the future
  3. I thought it was bad practice for functions to be able to modify system files once they are copied into the system. From that perspective I would worry about this not passing a CRAN review.

I'm willing to implement and properly test your suggestion, but just wanted to bring these up.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

The idea would be to change the other install_ functions to pass their remote_metadata calls to install, everything would use the same path, so nothing would need to be special cased.

  1. Maybe relevant, but as long as it doesn't generate a NOTE from R CMD check it should be ok.
@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

OK, I like that idea even better.

Final point, add to this pull request, or start over and submit a new one?

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

Sorry I was not more clear in my proof on concept what I had in mind.

I would keep this pull request. Actually I would have kept the previous one open so that the discussion was all together, you can always

# reset state to previous state
git reset upstream/master
# make new commits
...
git push --force

To remove the old commits if needed.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

So currently, the remote_metadata is an S3 function, with methods for github_remote, local_remote, etc. So I would see adding a new method for package, and then have something like:

install <- functioin(...., metadata = git_committed){

if (is.function(metadata)) {
  metadata <- git_committed(pkg$path)
} 

R CMD stuff

if (metadata) {
  new_metadata <- remote_metadata(pkg)
  add_metadata(base::system.file(package = pkg$package), new_metadata)
}

Does that seem reasonable? I'll start actually coding something up.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

I would really follow my proof on concept. Just write a new method for package

remote_metadata.package <- function(x, bundle = NULL, source = NULL) {
  list(
    RemoteType = "package",
    RemoteUrl = x$path,
    RemoteSha = if (git_committed(x$path)) git_sha1(path = x$path)
  )
}

Make the default argument for install metadata = remote_metadata(as.package(pkg)) and add it after the R CMD check.

if (length(metadata)) {
  add_metadata(base::system.file(package = pkg$package), metadata)
}

Then just change https://github.com/hadley/devtools/blob/master/R/install-remote.R#L20-L25 to pass the metadata to install rather than adding it beforehand.

diff --git a/R/install-remote.R b/R/install-remote.R
index b1192b4..a22232a 100644
--- a/R/install-remote.R
+++ b/R/install-remote.R
@@ -17,12 +17,13 @@ install_remote <- function(remote, ..., quiet = FALSE) {
   source <- source_pkg(bundle, subdir = remote$subdir)
   on.exit(unlink(source, recursive = TRUE), add = TRUE)

-  add_metadata(source, remote_metadata(remote, bundle, source))
-
   # Because we've modified DESCRIPTION, its original MD5 value is wrong
   clear_description_md5(source)

-  install(source, ..., quiet = quiet)
+  install(source,
+    metadata = remote_metadata(remote, bundle, source),
+      ..., quiet = quiet)
+
 }

You actually don't have to change any of the install_ functions directly.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

jimhester@212d272 has a working implementation passing the regular devtools tests (https://travis-ci.org/rmflight/devtools/builds/102617084#L2277-L2279). It just needs documentation and some additional tests written if possible.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

OK, trying it out, and there is an issue. packageDescription (which session_info uses to get package informationdoes not read from DESCRIPTION directly, but rather readspackage.rds, which looks like it is generated at INSTALL. This gets read byreadRDS`

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

I've got an implementation, but the test indicates that it doesn't work at modifying the information that shows up in session_info, which is what I think has to happen for this to be useful.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

As I noted, 70b9b25#diff-17d36a2a2b0eac6b55b22dae5787dc48R167, this test fails because packageDescription uses readRDS. So we either need to change the add_metadata to modify both the DESCRIPTION and the package.rds file, or change session_info to use read.dcf in the package location.

Do you or @hadley know offhand if there is a function for writing RDS files outside of what INSTALL does?

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

just found it, looks like saveRDS

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

I also think I might need to create some tests just for metadata checking

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

Mmm yes that is an issue, see jimhester@bd0cff1 for code which updates it as well as the DESCRIPTION.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

OK, this works (rmflight/devtools@9c7447a), and passes all of the previous devtools tests, and passes the new ones I wrote to check that the SHA1 is being written in.

@@ -46,3 +46,13 @@ remote_metadata.local_remote <- function(x, bundle = NULL, source = NULL) {
RemoteSha = if (uses_git(x$path)) git_sha1(path = x$path)
)
}
#' @export
remote_metadata.package <- function(x, bundle = NULL, source = NULL) {

This comment has been minimized.

@jimhester

jimhester Jan 15, 2016

Member

If we change this to the following it will give us consistent results with the legacy behavior if the package is not using git or has an unclean working directory.

compact(list(
    RemoteType = "local",
    RemoteUrl = x$path,
    RemoteSubdir = x$subdir,
    RemoteSha = if (git_committed(x$path)) git_sha1(path = x$path)
  ))

This comment has been minimized.

@jimhester
@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

Ok this is looking good, I think we should modify https://github.com/hadley/devtools/blob/81dd313e1d143a3bdaaa2e1f09c887801db66694/R/session-info.r#L156-L159 to work with this situation better. Right now the code assumes there is always a RemoteUsername and RemoteRepo if there is a RemoteType, which in this case (and when using install_local`) that is not true.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

In for a penny, in for a pound. May as well make it consistent somewhere else.

@hadley

This comment has been minimized.

Member

hadley commented Jan 15, 2016

It would be super useful if you could also fill in the github info if the repo has been pushed to github. (That would make my life easier when working with shinyapps and development versions of my packages)

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

That sounds really useful and cool @hadley, but can we maybe make an issue and assign it to me, and I can work on that particular feature some other time?

I want to get the reporting in session_info cleaned up, and then have this as a stand alone pull request and be done with it at this point.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

ok, rmflight/devtools@bb9e337 has a modified session_info that will generate:

from github:
categoryCompare2 * 0.99.158 2016-01-15 Github (rmflight/categoryCompare2@0478ba3)

from local git with clean tree and metadata not NULL:
testMetadataInstall * 0.0.0.9000 2016-01-15 package (@373e621)

and then local git with a dirty working directory:
testMetadataInstall * 0.0.0.9000 2016-01-15 package

does that work for you @jimhester ?

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

This should fill in the github user/repo information if the repository has a github remote set up. It doesn't necessarily mean the given revision is actually pushed to that remote however...

diff --git a/R/install-local.r b/R/install-local.r
index 6877f5d..b7d843e 100644
--- a/R/install-local.r
+++ b/R/install-local.r
@@ -49,10 +49,18 @@ remote_metadata.local_remote <- function(x, bundle = NULL, source = NULL) {

 #' @export
 remote_metadata.package <- function(x, bundle = NULL, source = NULL) {
-  list(
-    RemoteType = "package",
-    RemoteUrl = x$path,
-    RemoteSubdir = x$subdir,
-    RemoteSha = if (git_committed(x$path)) git_sha1(path = x$path)
+  res <- list(
+    RemoteType = "local",
+    RemoteUrl = x$path
   )
+
+  if (git_committed(x$path)) {
+    res$RemoteSha <-  git_sha1(path = x$path)
+  }
+  if (uses_github(x$path)) {
+    info <- github_info(x$path)
+    res$RemoteUsername <- info$username
+    res$RemoteRepo <- info$repo
+  }
+  res
 }
@jimhester

This comment has been minimized.

Member

jimhester commented Jan 15, 2016

Also I am not sure having a RemoteType = 'package' is useful, which is why I switched it back to 'local' in the diff.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 15, 2016

OK, fair enough on the package vs local.

If changing this, I feel like remote_metadata.local_remote and remote_metadata.package should be doing the exact same thing as far as annotating the metadata.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 20, 2016

If the two functions are literally the same I think we should just make them aliases.

remote_metadata.package <- remote_metadata.local_remote

Also could you rebase this on the current master and possibly squash some of the more trivial commits. https://help.github.com/articles/about-git-rebase/ has some help on using Git rebase if you need a reference.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 20, 2016

OK. Originally went with two functions so it would be easier to change in the future if decided they should be different, but you are right, currently they are the same, so should just alias one from the other.

I will give rebasing an attempt, this is the first time I've tried to use it.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 20, 2016

Just remember you can always abort the rebase with git rebase --abort if needed. If you are worried about messing something up you can checkout the current state into a new branch, then switch back. The backup won't be altered by the rebase.

git checkout install_sha
git checkout -b backup
git checkout install_sha
@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 20, 2016

Thanks, that helps.

rmflight added some commits Jan 20, 2016

add explanation of the formatting we are trying to achieve, and put t…
…he setting to NULL in else statements to make it a little clearer

@hadley hadley added this to the 1.10 milestone Jan 20, 2016

@hadley hadley self-assigned this Jan 20, 2016

@hadley

This comment has been minimized.

Member

hadley commented Jan 20, 2016

I don't think the logic is quite right. When I run install() with this version (on devtools itself), the installed DESCRIPTION is not modified, and the DESCRIPTION in the current directory is.

@hadley

This comment has been minimized.

Member

hadley commented Jan 20, 2016

The problem is that base::find.package(pkg$package) doesn't give the installed path for a package loaded with devtools, but instead gives the path to the source package. This will only be a problem if you've run load_all() before install(), but that seems like a fairly common scenario to me (and is certainly something I'd do)

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 21, 2016

Is there an easy way to tell that it was done by load_all so as to avoid
this? Or maybe we can look for the package in the directory specified by
.libPaths()?

On Wed, Jan 20, 2016, 5:56 PM Hadley Wickham notifications@github.com
wrote:

The problem is that base::find.package(pkg$package) doesn't give the
installed path for a package loaded with devtools, but instead gives the
path to the source package. This will only be a problem if you've run
load_all() before install(), but that seems like a fairly common scenario
to me (and is certainly something I'd do)


Reply to this email directly or view it on GitHub
#1027 (comment).

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 21, 2016

@rmflight Best thing to do is just use inst() here instead of find.package(). It will always give the path to the installed copy of the package.

See jimhester@fc359c2

use devtools::inst() to make sure we always modify the **installed** …
…version of the package, and add a test using load_all() followed by install as this was causing a failure previously
@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 21, 2016

Used inst() in 1c3fccd, and added a test of load_all(); install() to double check that the correct instance was indeed modified.

@jimhester

This comment has been minimized.

You should call on.exit(unload(test_pkg), add = TRUE) to clean up and unload the package, do so after loading it the first time at L40.

Also I would suggest you use withr::with_temp_libpaths() to set a temporary libPaths() rather than rolling your own at L27.

This comment has been minimized.

Owner

rmflight replied Jan 21, 2016

Re: temporary libPaths(): I just followed the examples I saw in other test-* files. I see this pattern, and don't see with_temp_libpaths() anywhere else in the devtools directory.

This comment has been minimized.

jimhester replied Jan 21, 2016

Well we added it to withr not too long ago, but no reason not to use it now!

@hadley

This comment has been minimized.

Member

hadley commented Jan 21, 2016

Ok - this now just needs a bullet in NEWS.md and we're good to merge.

@jjallaire @kevinushey FYI: this PR will add git/github information when installing from a local package. I vaguely remember there's some interaction with packrat, so you need to know about this.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 21, 2016

@jimhester: using withr::with_temp_libpaths() in the test suite 2f54a5e

@hadley: bullet added to News.md 526db44

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 21, 2016

OK, I'm curious, how do I resolve a conflict that I can't see?

@hadley hadley merged commit 526db44 into r-lib:master Jan 21, 2016

1 check failed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
@hadley

This comment has been minimized.

Member

hadley commented Jan 21, 2016

Don't worry about it, I just merged by hand.

@rmflight

This comment has been minimized.

Contributor

rmflight commented Jan 21, 2016

Thank you! And thank you and @jimhester for all of your support and coaching through this process. Just so you know, this was my first pull request that involved any amount of real code, and you both made it a pretty cool experience.

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