Lockbox Recursive Dependency Update #73
Conversation
|
Miniature version of test (using just 1 random CRAN package right now along with 10 from syberia-verse...still not a super-fast test) currently passes using devtools::test() but fails using devtools::check(). I'm definitely missing something obvious about the environment R CMD check produces. Tried to reproduce it by starting R with "R_DEFAULT_PACKAGES= LC_COLLATE=C R" but test() is still successful. If anyone has any ideas let me know. I'll try to debug. |
|
Found a workaround here: r-lib/testthat#86 |
|
Weirdly, the workaround fixes the check() run locally but not on Travis. Good times. |
|
@robertzk @peterhurford So I think doing X random CRAN packages is kind of a bust. Just look at the the after failure portion of this Travis build to see what I mean: https://travis-ci.org/robertzk/lockbox/builds/114380263 RODBC has outside R dependencies, and fails when installing from source. There are no binaries available for it (it requires compilation), and iODBC is required to be downloaded for installation: http://stackoverflow.com/questions/26210317/installation-of-rodbc-roracle-packages-on-os-x-mavericks I suspect there are waaaaay more packages like this that require hands-on intervention, and I don't think it's feasible to expect dependency installation that can deal with outside R dependencies, especially when considering that there are Bioconductor dependencies mixed in with some of these CRAN packages and I agree with Peter that lockbox should stop at those packages rather than issue a warning. Currently I've made it so that even when lockbox isn't in verbose mode it is still informative about these kinds of failures, and I honestly think that should be enough for most people. The fact that it will attempt a binary install if the source fails should catch a lot of problems as well, but there is only so much that can be done. I can hardcode in a yml of some CRAN packages for a test that passes Travis, but I think a random selection is out. If this is too much stochastic weirdness for lockbox then I can just keep everything in my fork, which I am comfortable doing. |
|
@davidboren Still sounds like you learned a lot and improved the code. How about you do a static test from a static lockfile? |
|
Agreed. Will just hardcode the cran yml |
|
@peterhurford Okay, as far as I'm concerned this is as ready to go as it's going to be. Using 10 syberia and 5 cran packages in the test (which is still not by any means a short test), and we're passing with no trouble. Look over anything you want and let me know. |
|
@davidboren Looks good. The test is great. The code could be cleaner but that sounds hard and we can refactor in a later PR... The only other thing worth revisiting is maybe squishing the commit history up. That makes me nervous but if you feel confident doing it, that would be nice. Otherwise I'm good to merge. |
|
Read through the full conversation. I'm happy with the compromises. Agree that packages that require non-R dependencies or do funky things in the build process should be exempt from the end-to-end test. I'll merge this now. |
|
Actually let me look over the new changes first. |
| if (is.null(ref)) arguments <- list(package$repo) | ||
| else arguments <- list(paste(package$repo, ref, sep = "@")) | ||
|
|
||
| token <- Sys.getenv("GITHUB_PAT") |
There was a problem hiding this comment.
Can you use github_pat() for this please?
|
Few minor comments, otherwise looks excellent. 💯 |
|
Definitely wait until it passes the test again, but I've taken care of those last comments. I've got to run. Picking up a drugged up dog from surgery. Will check back in later this evening in case something cropped up. |
|
Oh my bad... I think |
|
I just made the helper function. Test is green again after I broke things with said helper function. So the test is useful. I think I've addressed the comments. Should I squash this thing? Let me know. |
Lockbox Recursive Dependency Update
Okay! This thing is ready for code review, though probably not merging. Most of the relevant code is found in dependencies.R, since I've tried to minimize changes to the existing functions and files.
This update recursively parses description files, downloading them if necessary, and sorts both dependencies and locked packages in the same list. Dependencies are installed with locked packages in the same manner with the same function call, though dependencies will use the latest available version, only updating if packages' description file has updated it's version requirement beyond the one currently available in /.R/lockbox/
This recursively parsed dependency information is NOT stored anywhere, but the parsing is done each time. With a big lockfile it tends to add less than 2 seconds to startup, and certainly much less than the overall loading time. Of course, when the packages have to be downloaded, this parsing can take longer, but still much less time than installation time. There is some repetition of downloading here, so if we are unhappy with loading time on new package installations we can always add some additional logic to save paths to downloaded files
Currently, in the interest of leaving the original code as untouched as possible, I've left a little bit that could be dried up if combined with previous code, especially that in library.R. The download_package.CRAN function shares some code with install_old_CRAN package, so that can be dried up if we want.
I'm accessing the Remotes portion of description files as well, but final dependencies are decided based on version comparison alone, since there is no real good way to compare two different forks of the same repo. In addition, we currently only support github remotes, since that follows from the rest of lockbox.
Two private devtools functions are currently accessed in order to save space, remote_download.github_remote and github_remote, but we can import them in explicitly if we want to be safer. At that point, however, I might just recommend dropping the devtools dependency entirely from lockbox, since we're just downloading all the same stuff in the same way. Then we could also lock in devtools :) I've also stolen the tools::package.dependencies function and updated it a bit so it doesn't break and gives us remotes back (this is the biggest function in dependencies.R....I hates it but don't want to touch it any more)
I've run devtools::document, so we've got a lot of extra files, but I've only altered by hand the existing files library.R, lockbox.R, and align.R (align is only changed to reduce the pesky_namespaces to lockbox's dependencies).
You'll find some logic that allows for NA versioning, which is acceptable for packages that have no version requirements. Prior to installation they are updated with either the latest remote/CRAN version available (if we do not have the dependency or require a newer version) or the newest version in the lockbox. Some logic in lockbox.R is updated for this reason and also to distinguish, in super pretty crayon fashion, a dependency load or installation from a lockfile load or installation.
Finally, you should note that it's taken me more commits to get this right than it took for RobK to write the package in the first place. Go me!