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

Fix revdep_check() when git2r is not loaded #1068

Merged
merged 4 commits into from
Feb 11, 2016
Merged

Fix revdep_check() when git2r is not loaded #1068

merged 4 commits into from
Feb 11, 2016

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Feb 4, 2016

When installing package during revdep check, use new libpath in addition to existing libpath. Reason: if git2r is not loaded yet, an error is raised if it not in the temporary library.

@jimhester
Copy link
Member

This LGTM. I ran into this as well. I think this is a regression caused by the switch to withr. withr::with_libpaths has a default action of 'replace', while I believe the devtools version prepended.

@krlmlr
Copy link
Member Author

krlmlr commented Feb 4, 2016

There are two more instances of with_libpaths():

R/check-cran.r:51:  libpaths_orig <- withr::with_libpaths(libpath, {
R/build.r:72:  withr::with_libpaths(c(temp_lib, .libPaths()), R(cmd, path, quiet = quiet))

@jimhester
Copy link
Member

I was wrong about it being a regression of set_libpaths, it did replace completely previously.

That first instance was deliberately changed to from prepending to replace at
516e59f. The second looks like we can replace with action = 'prefix'.

@krlmlr
Copy link
Member Author

krlmlr commented Feb 9, 2016

Merged with master, and replaced instance of with_libpaths() by with_temp_libpaths().

@@ -118,10 +118,11 @@ revdep_check <- function(pkg = ".", recursive = FALSE, ignore = NULL,
dir.create(srcpath)

message("Installing ", pkg$package, " ", pkg$version, " from ", pkg$path)
withr::with_libpaths(libpath, install(pkg, reload = FALSE, quiet = TRUE))
withr::with_libpaths(libpath, action = "prefix",
install(pkg, reload = FALSE, quiet = TRUE))
Copy link
Member

Choose a reason for hiding this comment

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

I think this maybe needs dependencies = TRUE

Kirill Müller added 4 commits February 11, 2016 14:17
Reason: if git2r is not loaded yet, an error is raised if it not in the temporary library.
checking reverse suggests would probably install the suggests anyway
@krlmlr
Copy link
Member Author

krlmlr commented Feb 11, 2016

Restored lines killed when resolving merge conflicts, added "dependencies = TRUE".

hadley added a commit that referenced this pull request Feb 11, 2016
Fix revdep_check() when git2r is not loaded
@hadley hadley merged commit d489df9 into r-lib:master Feb 11, 2016
@hadley
Copy link
Member

hadley commented Feb 11, 2016

BTW I think if you always add the NEWs bullet at the top, it's less likely to cause merge conflicts because it gets anchored to the unchanging heading. (That's my theory anyway)

@krlmlr krlmlr deleted the feature/revdep-git2r branch February 11, 2016 13:57
@krlmlr
Copy link
Member Author

krlmlr commented Feb 11, 2016

Nope: I have put together a test script that simulates adding a bullet point to the top of the NEWS file, a merge conflict is raised when trying to merge the second branch. Using .gitattributes helps, but only locally, not on GitHub.

In my code, I record the NEWS entries in the bodies of the merge commit messages. Then, just before release, I use

git log --first-parent <tag>.. --format=format:"%b" | sed "/^$/d"

where <tag> is the last released tag. The log command lists precisely the merge commits, the sed removes empty lines.

@jimhester
Copy link
Member

setting NEWS.md merge=union in the projects .gitattributes works locally, but GitHub ignores that setting when doing its merge. That is unfortunate 😦. It is probably still worth setting it for devtools as it would make local merging easier. isaacs/github#487 is the un-official issue for GitHub support for union merges.

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