inbo / checklist

An R package for checking R packages and R code
https://inbo.github.io/checklist
GNU General Public License v3.0
16 stars 2 forks source link

switch from git2r to gert #68

Closed hansvancalster closed 2 years ago

hansvancalster commented 2 years ago

Most functions and corresponding tests now use gert instead of git2r functions. This should allow both https- and ssh-protocol for communication with Git and GitHub.

In check_description.R on lines 88-91 I have left some lines of code which still use git2r functions because I could not find a gert alternative (yet). Also in check_filename.R on line 53, I haven't found a good alternative for git2r::in_repository().

hansvancalster commented 2 years ago

When I run checklist::check_package() locally, all is OK and all tests pass. They fail here and I suspect this has to do with GHA workflows specifying master instead of main as main branch (? - just a feeling)

hansvancalster commented 2 years ago

When I run checklist::check_package() locally, all is OK and all tests pass. They fail here and I suspect this has to do with GHA workflows specifying master instead of main as main branch (? - just a feeling)

On second thought, it's probably because these are breaking changes. Checking the checklist package in this branch with current checklist version on main branch will fail. Locally I check the package with a fresh built of the package that's being checked 🤯

hansvancalster commented 2 years ago

On second thought, it's probably because these are breaking changes. Checking the checklist package in this branch with current checklist version on main branch will fail. Locally I check the package with a fresh built of the package that's being checked 🤯

I just did remotes::install_github("inbo/checklist") and then ranchecklist::check_package()` and this returned only a couple of lintr issues (which all look like false positives to me; I need to stop can't think straight...):

############################################################################################################################
14 linters found.
`styler::style_file()` can fix some problems automatically. 

----------------------------------------------------------------------------------------------------------------------------
6 times "no visible global function definition for ‘git_status’"
2 times "no visible global function definition for ‘git_branch_checkout’"
2 times "no visible global function definition for ‘git_branch_list’"
1 times "no visible global function definition for ‘git_branch_delete’"
1 times "no visible global function definition for ‘git_fetch’"
1 times "no visible global function definition for ‘git_ls’"
1 times "possible error in clean_git(repo = repo, verbose = verbose): unused argument (repo = repo)"

############################################################################################################################

Checking the package revealed some problems.
ElsLommelen commented 2 years ago

Just from the errors itself, it seems you might have forgotten to add these functions as dependencies. And last error: probably argument repo doesn't exist in clean_git()

hansvancalster commented 2 years ago

Just from the errors itself, it seems you might have forgotten to add these functions as dependencies. And last error: probably argument repo doesn't exist in clean_git()

No that's not the problem. They are all loaded via importFrom and are present in the namespace and package gert is in description imports section. The last error is also false: there is an argument repo.

florisvdh commented 2 years ago

The first error on GHA, indicated at https://github.com/inbo/checklist/runs/3633245557#step:10:374, seems the cause of all subsequent errors:

 ── Error (test_b_clean_git.R:27:3): clean_git with `main` as main branch ───────
Error: Error: src refspec 'refs/heads/main' does not match any existing object

I didn't look into the test file (or setup for it), but this looks like branch misnaming indeed, or perhaps worse (not running in a git repo, or forgot to fetch?).

ThierryO commented 2 years ago

In check_description.R on lines 88-91 I have left some lines of code which still use git2r functions because I could not find a gert alternative (yet). Also in check_filename.R on line 53, I haven't found a good alternative for git2r::in_repository().

I don't like to import two packages with very similar functions into the same package. That makes things confusing.

ThierryO commented 2 years ago

The gert:::raise_libgit2_error seems to suggest that the libgit2 linux library is not available.

You need to add gert to the Dockerfile. Untill the release with gert you'll need to add the required aptget to the GHA.

hansvancalster commented 2 years ago

The gert:::raise_libgit2_error seems to suggest that the libgit2 linux library is not available.

You need to add gert to the Dockerfile. Untill the release with gert you'll need to add the required aptget to the GHA.

From https://github.com/r-lib/gert#installation it seems I need to do the following for Ubuntu Trusty and Xenial:

sudo add-apt-repository ppa:cran/libgit2
sudo apt-get update
sudo apt-get install libgit2-dev

The aptget already has libgit2-dev and runs on ubuntu-latest. Do you think I need to change something? Something like:

on: [push]

name: check-pkg

jobs:
  check_package:
    runs-on: ubuntu-latest
    name: Check package
    steps:
      - uses: inbo/actions/check_pkg@master
        with:
          - path: my_path
          - CODECOV_TOKEN: "some codecov token"
          - ORCID_TOKEN: "my ORCID token"
          - add-apt-repository: ppa:cran/libgit2 #syntax correct? colons in the correct place?
          - apt-get: update #syntax correct? colons in the correct place?
          - apt-get: install libgit2-dev #syntax correct? colons in the correct place?
ThierryO commented 2 years ago

You need to provide a comma separated list of apt-get package to install. Custom ppa are currently not supported.

on: [push]

name: check-pkg

jobs:
  check_package:
    runs-on: ubuntu-latest
    name: Check package
    steps:
      - uses: inbo/actions/check_pkg@master
        with:
          - path: my_path
          - CODECOV_TOKEN: "some codecov token"
          - ORCID_TOKEN: "my ORCID token"
          - apt-get: libgit2-dev
hansvancalster commented 2 years ago
############################################################################################################################
14 linters found.
`styler::style_file()` can fix some problems automatically. 

----------------------------------------------------------------------------------------------------------------------------
6 times "no visible global function definition for ‘git_status’"
2 times "no visible global function definition for ‘git_branch_checkout’"
2 times "no visible global function definition for ‘git_branch_list’"
1 times "no visible global function definition for ‘git_branch_delete’"
1 times "no visible global function definition for ‘git_fetch’"
1 times "no visible global function definition for ‘git_ls’"
1 times "possible error in clean_git(repo = repo, verbose = verbose): unused argument (repo = repo)"

############################################################################################################################

Checking the package revealed some problems.

The issue with the false positive lintrs is gone after 1563e1e (removing redundant gert:: ; which is really strange: apparently lintr throws a bunch of unrelated issues. It probably silently assumes that one does not use function:: in a redundant way?)

hansvancalster commented 2 years ago

The gert:::raise_libgit2_error seems to suggest that the libgit2 linux library is not available.

You need to add gert to the Dockerfile. Untill the release with gert you'll need to add the required aptget to the GHA.

This was not necessary. The problem was that tests failed in GHA because in that environment the main branch was called master whereas when I ran the tests locally, the main branch was called main. This is now fixed by setting the main branch name programmatically in the tests.

hansvancalster commented 2 years ago

One of the tests in test_b_clean_git.R still fails in GHA. When I run it locally everything is fine. Can you have a look at this one failing test, because I have no clue what's going on.

hansvancalster commented 2 years ago

Commit a1ac0f2 needs careful review, because I was not sure from the documentation of git2r::diff.git_tree if my git shell commands are entirely equivalent. In the code I am not diffing trees but diffing the commits that go with the trees (which should be OK I think?). I've used the git diff <commit>..<commit> -- [<path>] syntax explained here.

ThierryO commented 2 years ago

@hansvancalster Feel free to add your name as contributor to the DESCRIPTION.