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 PEcAn packages before resolving dependencies #1556

Merged
merged 1 commit into from
Jul 31, 2017

Conversation

infotroph
Copy link
Member

@infotroph infotroph commented Jul 30, 2017

Prevent build errors by ensuring that all PEcAn packages are installed before devtools::install_deps is invoked

Description

Motivation and Context

Make was failing after clean R installations, but succeeding when older versions of all dependency packages were available. There appear to be two simultaneous issues here:

  • the Makefile's dependency lists for Biocro and uncertainty were out of date*
  • devtools::install_deps tries to resolve dependencies by installing them from one specified repository, so it doesn't know what to do with a package that Depends on, say, both PEcAn.db (local) and ncdf4 (CRAN).

Switching the all recipe from document install to install document seems to make everything happen in the correct order on my system, but please check my assumptions.

*P.S. While I'm looking at Make dependencies, I suspect the BioCro line is in practice acting as the depends for all the models -- all of them at least import PEcAn.util, but most models have no dependency line in the Makefile. If the current arrangement works, is there any problem with that?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@mdietze mdietze left a comment

Choose a reason for hiding this comment

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

Not an ideal fix, as any documentation updates won't get into the build, but happy to have a way to get things working again

@mdietze mdietze merged commit 9d48146 into PecanProject:develop Jul 31, 2017
@ashiklom
Copy link
Member

The reason document was before install is that document updates the NAMESPACE file based on the roxygen @export statements, which defines which functions are available when the package is loaded. If people are diligent about running document locally before pushing, it shouldn't matter.

More generally, I think the correct way to address the dependencies problem is to add remotes to the DESCRIPTION file, as described in the remotes package vignette (https://github.com/hadley/devtools/blob/master/vignettes/dependencies.Rmd).

@mdietze
Copy link
Member

mdietze commented Jul 31, 2017

The number of PR that fail because of the documentation check sure suggests folks are not yet diligent about running document locally before pushing -- while annoying, I really appreciate that check.

In terms of adding remotes to the DESCRIPTION, I think that's a great idea. @ashiklom could you open an Issue about adding that to existing packages and @tonygardella could you add that to our development guidelines in the documentation?

@mdietze
Copy link
Member

mdietze commented Jul 31, 2017

@ashiklom one possible problem with the use of Remotes in the packages -- if these are called first would that result in PEcAn packages being downloaded directly from github and built based on the latest stable release code rather than the code residing locally on the machine itself? This sounds like and advantage for anyone trying to use a single PEcAn package as a stand-alone tool, but could be a real pain for developers and might lead to both odd errors and long builds (if make install then rebuilds every PEcAn package a second time using the local code).

@ashiklom
Copy link
Member

@mdietze I've thought about this too. My understanding is that the remotes package first considers package versions, then differences in SHA hashes. Since we never update the version numbers of PEcAn packages (though perhaps we should) nor specify them in dependencies, I think the remotes package will only try to install PEcAn packages if there isn't any version of the package available. I think this means that our current build process shouldn't need to be changed or take any longer because the makefile is already set up to install in an order that satisfies the PEcAn dependencies of every package.

@ashiklom
Copy link
Member

ashiklom commented Jul 31, 2017

Actually, scratch that -- yes, if we install dependencies first, the build process will in fact be longer because we install the dependencies and then re-install the local packages.

I think the way to deal with that is to adjust the make build process so it operates package-by-package rather than all at once -- i.e. document and install one package, then document and install another, etc., instead of documenting all the packages and then installing them.

@mdietze
Copy link
Member

mdietze commented Jul 31, 2017

@ashiklom will that apply to every local build, or just the first build of each package? (the latter is obviously more tolerable). Will it apply to every Travis build?

@ashiklom
Copy link
Member

See #1557 and/or #1558 .

I think only the first build should take a long time; after that, the build will already have all of the dependencies available, so it should only reinstall the packages.

Locally, I think that's pretty robust, as long as people have their R library paths, etc. set up correctly.

On Travis, it depends on the cache. As long as the previous cache is available, it's effectively the same as a local build -- all dependencies that have been installed by a previous build will be available, so the build should be fairly fast. That said, our current Travis build doesn't have much room to be any longer (it already tends to run up against the build time limit), so the odds of a build from scratch timing out before finishing are reasonably high. And, by design, Travis only stores caches from completely successful builds, so even minor errors at the very end of a build (e.g. forgetting to make document, even if it runs fine on Travis) will require the entire thing to run again from scratch.

@infotroph infotroph deleted the make-depends-fix branch July 31, 2017 13:56
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.

None yet

3 participants