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

Setting working dir correctly for git2r commits #70

Merged
merged 4 commits into from Jan 31, 2018

Conversation

Projects
None yet
2 participants
@nfultz
Copy link
Contributor

commented Jan 19, 2018

Hi Dirk, I'm trying to get appveyor to push windows zips for DeclareDesign, we had been getting clean builds but containing

Added and committed randomizr_0.7.0.zip plus PACKAGES files. Still need to push.
Warning message:
In git2r::commit(repo, msg) :
  Error in 'git2r_commit': Nothing added to commit

at then end - (I had't caught this before christmas because many of our changes to master at that time were for pkgdown websites, which trigger builds but the .zip doesn't change and git would correctly not record a change).

Anyway, I spent some time debugging, and finally noticed that on the git2r side, we both setwd() to 'bin/windows/contrib/3.4' and then also add(repo, "bin/windows/contrib/3.4/randomizr_0.8.0.zip"), so git2r::add tries to normalize the path and ultimately add bin/windows/contrib/3.4/bin/windows/contrib/3.4/randomizr_0.8.0.zip

Unclear to me why this is broken for appveyor/windows and not for travis/linux, but changing the two lines in this PR seems to fix it. Probably git2r has changed since these lines were written ?

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Jan 19, 2018

Hm. It works here. I mostly do this from the command line and I don't always push. But see

~/git/drat-test-repo$ git init              ## create repo
Initialized empty Git repository in /home/deddelbuettel/git/drat-test-repo/.git/
~/git/drat-test-repo(master)$ git checkout -b gh-pages
Switched to a new branch 'gh-pages'
~/git/drat-test-repo(gh-pages)$ touch foo
~/git/drat-test-repo(gh-pages)$ git add foo 
~/git/drat-test-repo(gh-pages)$ git commit -m '123'    ## have a fist commit
[gh-pages (root-commit) 0f933e9] 123
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 foo
~/git/drat-test-repo(gh-pages)$ cd ..
~/git/drat-test-repo(master)$ cd ..
~/git$ build.r drat                                               ## build a tar ball
* checking for file ‘drat/DESCRIPTION’ ... OK
* preparing ‘drat’:
* checking DESCRIPTION meta-information ... OK
* installing the package to build vignettes
* creating vignettes ... OK
* checking for LF line-endings in source and make files and shell scripts
* checking for empty or unneeded directories
* building ‘drat_0.1.4.tar.gz’

~/git$ dratInsert.r -c '"some silly message"' -r drat-test-repo/ drat_0.1.4.tar.gz 
Added and committed drat_0.1.4.tar.gz plus PACKAGES files. Still need to push.

~/git$ 

But it is likely that platform specfic code does something wrong. I'll look at the PR.

@@ -96,7 +96,7 @@ insertPackage <- function(file,
pkgtype <- identifyPackageType(file)
reldir <- getPathForPackage(file)

pkgdir <- file.path(repodir, reldir)
pkgdir <- normalizePath(file.path(repodir, reldir))

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Jan 19, 2018

Owner

That part is fine.

@@ -116,7 +116,7 @@ insertPackage <- function(file,
if (commit) {
if (haspkg) {
repo <- git2r::repository(repodir)
setwd(pkgdir)
setwd(repodir)

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Jan 19, 2018

Owner

This I have a problem with. I think it needs to be pkgdir, ie what we compute above.

And I just used that for a Windows package a couple of days ago.

This comment has been minimized.

Copy link
@nfultz

nfultz Jan 19, 2018

Author Contributor

Also notice the file.path(reldir) in the add calls below, it doesn't quite match the shell commands in the non-git2r half of the function.

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Jan 20, 2018

Owner

Could you be more explicit, possibly with a worked example.

Burden of proof is on you. The package works for me and other as is, and as I outlined the suggested change here is simply not good in my book and will block this PR forever unless removed. Or you make me change my mind.

This comment has been minimized.

Copy link
@nfultz

nfultz Jan 20, 2018

Author Contributor

the logic in MWE would be equivalent to the below psuedocode:

if haspkg then
  cd repo/bin/windows/contrib/3.4/
  git2r add bin/windows/contrib/3.4/drat_0.1.4.zip
  git2r add bin/windows/contrib/3.4/PACKAGES
  git2r add bin/windows/contrib/3.4/PACKAGES.gz 
  git2r add bin/windows/contrib/3.4/PACKAGES.rds 
  git2r commit
else
  cd repo/bin/windows/contrib/3.4/
  git add drat_0.1.4.gzip PACKAGES PACKAGES.gz PACKAGES.rds
  git commit
end if

where in the git2r branch the paths are prefixed with reldir but not in the git shell branch.

The git shell command will error (on linux) if you use the reldir prefix after cd-ing to it:

nfultz@neal-SH97:repo$cd bin/windows/contrib/3.4
nfultz@neal-SH97:3.4$touch drat_0.1.4.zip
nfultz@neal-SH97:3.4$git add bin/windows/contrib/3.4/drat_0.1.4.zip
fatal: pathspec 'bin/windows/contrib/3.4/drat_0.1.4.zip' did not match any files

I think the reldir prefix used to be required for git2r when those lines were written for drat, but since then git2r has added path normalization to git2r::add() to emulate the git shell command.

From what I can tell from debugging,
git2r::add() tries to normalize the path relative to the repo root, so if you setwd("bin/windows") then git2r::add("contrib/3.4/drat_0.1.4.zip") works correctly because git2r has R code to normalize the relative path to be relative to the repo root; however setwd("bin/windows"); git2r::add("bin/windows/contrib/3.4/drat_0.1.4.zip") would work on linux (and your windows environment but not mine/appveyor).

My initial suspicion was that windows \ seperators were breaking a regex inside git2r, but that apparently is not the case in my MWE.


Instead of changing setwd(pkgpath), would you be open to removing the reldir prefix in the git2r::add calls? I think that would make the two arms of the if statement more symmetric.

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Jan 20, 2018

Owner

I am not sure how much time and interest I have for setting up test cases.

AFAICT there is still nothing broken. You seem annoyed that files are prefixed in one case but not the other. I only care that the files end up in the git repo -- which appears to happen.

So I still do not see a need for change. I may be blind (and I am chasing another bug somewhere else but still...).

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Jan 19, 2018

Ditto for Windoze:

~/git/drat-test-repo(gh-pages)$ mkdir -p bin/windows/contrib/3.4
~/git/drat-test-repo(gh-pages)$ cd ..
~/git$ wget https://cloud.r-project.org/bin/windows/contrib/3.4/drat_0.1.4.zip
--2018-01-19 14:53:31--  https://cloud.r-project.org/bin/windows/contrib/3.4/drat_0.1.4.zip
Resolving cloud.r-project.org (cloud.r-project.org)... 13.33.164.216, 13.33.164.126, 13.33.164.218, ...
Connecting to cloud.r-project.org (cloud.r-project.org)|13.33.164.216|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 74781 (73K) [application/zip]
Saving to: ‘drat_0.1.4.zip’

drat_0.1.4.zip                                       100%[=====================================================================================================================>]  73.03K  --.-KB/s    in 0.003s  

2018-01-19 14:53:31 (22.8 MB/s) - ‘drat_0.1.4.zip’ saved [74781/74781]

~/git$ dratInsert.r -c '"adding win binary"' -r drat-test-repo/ drat_0.1.4.zip 
Added and committed drat_0.1.4.zip plus PACKAGES files. Still need to push.

~/git$ ls drat-test-repo/bin/windows/contrib/3.4/
drat_0.1.4.zip  PACKAGES  PACKAGES.gz  PACKAGES.rds
~/git$ 

dratInsert.r is a benign wrapper from here.

@nfultz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

Are you using the new version of git2r (0.21.0) ? Maybe something changed underneath drat.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Jan 19, 2018

Everything from CRAN is current.

@nfultz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

Here is my MWE on windows outside of appveyor triggering the weird error using CRAN git2r and drat:


install.packages("git2r")
install.packages("drat")

setwd(tempdir())

require(git2r)
require(drat)

dir.create("mydrat")
repo <- git2r::init("mydrat")
git2r::config(repo, user.name="Alice", user.email="alice@example.org")
repo <- git2r::repository("mydrat")
cat("Here I am", file="mydrat/README")
git2r::add(repo, "README")
my_initial_commit <- git2r::commit(repo, message="Initial Commit")
git2r::branch_create(my_initial_commit, name = "gh-pages")

download.file("https://cloud.r-project.org/bin/windows/contrib/3.4/drat_0.1.4.zip", "drat_0.1.4.zip")
drat::insertPackage("drat_0.1.4.zip", "mydrat", "my commit message")

This lead to -

> drat::insertPackage("drat_0.1.4.zip", "mydrat", "my commit message")
Added and committed drat_0.1.4.zip plus PACKAGES files. Still need to push.

Warning message:
In git2r::commit(repo, msg) :
  Error in 'git2r_commit': Nothing added to commit

Here is a short debugging session that shows that git2r is for some reason mangling the file name to add - https://gist.github.com/nfultz/f30d8ab3d8ceb6475454647137970c0a

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Jan 20, 2018

Did it, or didn't it, commit? Mine did:

~/git/drat-test-repo(gh-pages)$ git ls
* 936f8d0 - (HEAD -> gh-pages) adding win binary (3 hours ago) <Dirk Eddelbuettel>
* 32a9aa8 - some silly message (4 hours ago) <Dirk Eddelbuettel>
* 0f933e9 - 123 (4 hours ago) <Dirk Eddelbuettel>
~/git/drat-test-repo(gh-pages)$ 
@nfultz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2018

It did not commit :(

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Jan 20, 2018

Works here on Linux.

R> setwd(tempdir())
R> require(git2r)
Loading required package: git2r
R> require(drat)
Loading required package: drat
R> dir.create("mydrat")
R> repo <- git2r::init("mydrat")
R> git2r::config(repo, user.name="Alice", user.email="alice@example.org")
R> repo <- git2r::repository("mydrat")
R> cat("Here I am", file="mydrat/README")
R> git2r::add(repo, "README")
R> my_initial_commit <- git2r::commit(repo, message="Initial Commit")
R> git2r::branch_create(my_initial_commit, name = "gh-pages")
R> download.file("https://cloud.r-project.org/bin/windows/contrib/3.4/drat_0.1.4.zip", "drat_0.1.4.zip")
trying URL 'https://cloud.r-project.org/bin/windows/contrib/3.4/drat_0.1.4.zip'
Content type 'application/zip' length 74825 bytes (73 KB)
==================================================
downloaded 73 KB

R> drat::insertPackage("drat_0.1.4.zip", "mydrat", "my commit message")
Added and committed drat_0.1.4.zip plus PACKAGES files. Still need to push.

R> system("cd mydrat && git ls")
* 3baa26f - (HEAD -> gh-pages) my commit message (25 seconds ago) <Alice>
* 9697990 - (master) Initial Commit (36 seconds ago) <Alice>R> 
R> 

Shall we talk to Stefan? I am not sure this is a drat issue.

@nfultz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2018

I've tracked down the underlying issue to a behavior in git2r::add - https://github.com/ropensci/git2r/blob/master/R/index.R#L98

        np <- suppressWarnings(normalizePath(p, winslash = "/"))

where p is the file name passed in (eg "bin/windows/contrib/3.4/drat_0.1.4.zip")

On linux, normalizePath on a file it can't find ->

> p
[1] "bin/windows/contrib/3.4/drat_0.1.4.zip"
> normalizePath(p)
[1] "bin/windows/contrib/3.4/drat_0.1.4.zip"
Warning message:
In normalizePath(p) :
  path[1]="bin/windows/contrib/3.4/drat_0.1.4.zip": No such file or directory

On windows 10:

> p
[1] "bin/windows/contrib/3.4/drat_0.1.4.zip"
> normalizePath(p, winslash="/")
[1] "C:/Users/nfultz/AppData/Local/Temp/Rtmpw7D63e/mydrat/bin/windows/contrib/3.4/bin/windows/contrib/3.4/drat_0.1.4.zip"
Warning message:
In normalizePath(path.expand(path), winslash, mustWork) :
  path[1]="bin/windows/contrib/3.4/drat_0.1.4.zip": The system cannot find the path specified

git2r is suppressing those warnings in order to make globbing work, otherwise we would have noticed this sooner. It's somewhat accidental that linux is working in this case, to the extent that drat is relying on system-dependent error handling.

However, if drat set either the working directory or relative file path differently, normalizePath could return the canonicalized path, which git2r could then edit appropriately.

I'm going to continue using my patched drat for appveyor in the meantime. I still don't understand why windows worked for you - I've tried it on three different windows boxes as well as appveyor.

Thanks for your patience.

@nfultz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2018

On your Ditto for Windoze comment above - were you running that using a Windows box, or just adding a windows zip using a linux box ?

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Jan 20, 2018

Yes, sorry that was just for a Windows zip file, but inserting on a repo on a Linux partition.

I tend not to have easy access to Windows, but I have a dormant VM at work.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Jan 20, 2018

Good and patient work above (didn't see the earlier message at first). It is painful to work through all those cases but I guess we then want to maybe copy files "by hand" in R and then present a non-path file argument to git2r. We'll get there. At least for the time being you have a working version too.

I'll lean on you for some testing then.

@nfultz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

@eddelbuettel I've updated this PR to setwd(pkgdir), and also added an end-to-end test based on my MWE only using package.skeleton instead of downloading. After a day or so I should know if appveyor is working well with it or not.

@nfultz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2018

confirmed that we got a working windows build dratt'd.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Jan 31, 2018

Much appreciate your patience and follow-up. This looks clean, and tested fine for me too.

normalizePath() is a good addition, and the setwd() should help us as well. Nice PR :)

@eddelbuettel eddelbuettel merged commit 32ffe59 into eddelbuettel:master Jan 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.