lorenzwalthert / precommit

pre-commit hooks for R projects
https://lorenzwalthert.github.io/precommit/
GNU General Public License v3.0
252 stars 48 forks source link

Catch up with pre-commit.ci lite #450

Open lorenzwalthert opened 1 year ago

lorenzwalthert commented 1 year ago

https://github.com/pre-commit-ci/issues/issues/13

philipp-baumann commented 1 year ago

Hi @lorenzwalthert thanks a lot for providing these R hooks and excellent documentation! Not sure if this is the right place to address my current issue (it's just a gut feeling). I used pre-commit-ci-lite before for checking {opusreader2}, which sometimes failed because docopt for example could not be loaded (intermediarily fixed when skipping roxygenize like you recommended in docs). I changed to only precommit/action@v3.0.0 in main branch, which now does all the necessary code checks except roxygen2 and passes there. However, when I merged the fixed CI (current main) into a PR branch (mentioned above), the PR branch still fails (locally, everything passes). Do I manually need to delete caches in GH actions or do you maybe see anything is wicked in my setup?

# CI error https://github.com/spectral-cockpit/opusreader2/actions/runs/3768762143/jobs/6407435220
...
Run pre-commit run --show-diff-on-failure --color=always --all-files
[INFO] Initializing environment for https://github.com/lorenzwalthert/precommit.
[INFO] Initializing environment for https://github.com/lorenzwalthert/precommit:dplyr@1.0.9,git2r,r-lib/pkgapi,roxygen2@7.2.2.
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Installing environment for https://github.com/lorenzwalthert/precommit.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/lorenzwalthert/precommit.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/Rscript', '--no-save', '--no-restore', '--no-site-file', '--no-environ', '-e', '    options(install.packages.compile.from.source = "never")\n    renv::install(commandArgs(trailingOnly = TRUE))\n    ', 'git2r', 'r-lib/pkgapi', 'dplyr@1.0.9', 'roxygen2@7.2.2')
return code: 1
expected return code: 0
stdout: (none)
stderr:
    Error: failed to resolve remote 'r-lib/pkgapi' -- failed to retrieve 'https://api.github.com/repos/r-lib/pkgapi' [error code 22]
    In addition: Warning message:
    curl: (22) The requested URL returned error: 403 
...

Here is my current CI template (works in main but not in PR branch:

# .github/workflows/code-check.yml
name: Check code
on:
  pull_request:
  push:
    branches: [main]

jobs:
  main:
    runs-on: ubuntu-22.04
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-python@v4
        with:
          python-version: 3.x
      - uses: pre-commit/action@v3.0.0
      # - uses: pre-commit-ci/lite-action@v1.0.0
      #   name: pre-commit-ci-lite
        if: always()
# .pre-commit-config.yaml
# All available hooks: https://pre-commit.com/hooks.html
# R specific hooks: https://github.com/lorenzwalthert/precommit
repos:
  - repo: https://github.com/lorenzwalthert/precommit
    rev: v0.3.2.9005
    hooks:
      - id: style-files
        args: [--style_fun=tidyverse_style]
        exclude: .R/create_dataset.R
      - id: roxygenize
        additional_dependencies:
          - git2r
          - r-lib/pkgapi
          - dplyr@1.0.9
          - roxygen2@7.2.2
      # codemeta must be above use-tidy-description when both are used
      # - id: codemeta-description-updated
      - id: use-tidy-description
      - id: spell-check
        exclude: >
          (?x)^(
          .*\.[rR]|
          .*\.feather|
          .*\.jpeg|
          .*\.pdf|
          .*\.png|
          .*\.py|
          .*\.RData|
          .*\.rds|
          .*\.Rds|
          .*\.Rproj|
          .*\.sh|
          inst/extdata/.*|
          (.*/|)\.gitignore|
          (.*/|)\.gitlab-ci\.yml|
          (.*/|)\.lintr|
          (.*/|)\.pre-commit-.*|
          (.*/|)\.Rbuildignore|
          (.*/|)\.Renviron|
          (.*/|)\.Rprofile|
          (.*/|)\.travis\.yml|
          (.*/|)appveyor\.yml|
          (.*/|)NAMESPACE|
          (.*/|)renv/settings\.dcf|
          (.*/|)renv\.lock|
          (.*/|)WORDLIST|
          \.github/workflows/.*|
          data/.*|
          )$
      - id: lintr
        exclude: .R/create_dataset.R
      - id: readme-rmd-rendered
      - id: parsable-R
      - id: no-browser-statement
      - id: no-debug-statement
      - id: deps-in-desc
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.4.0
    hooks:
      - id: check-added-large-files
        args: ["--maxkb=200"]
        # -   id: file-contents-sorter
        files: '^\.Rbuildignore$'
      - id: end-of-file-fixer
        exclude: >
          (?x)^(
          \.Rd|
          \.csv
          )$
      - id: mixed-line-ending
        args: ["--fix=lf"] # use UNIX 'lf' character
        exclude: >
          (?x)^(
          \.Rd|
          \.csv
          )$
  - repo: local
    hooks:
      - id: forbid-to-commit
        name: Don't commit common R artifacts
        entry: Cannot commit .Rhistory, .RData, .Rds or .rds.
        language: fail
        files: '\.(Rhistory|RData|Rds|rds)$'
        # `exclude: <regex>` to allow committing specific files

ci:
  autoupdate_schedule: monthly
  skip: [roxygenize]

2022-12-24_01-06_pre-commit-run-all

Have you maybe experienced something like this already? Thanks a lot for a hint :-) Cheers, Philipp

philipp-baumann commented 1 year ago

hmm interesting all of the above is now automagically fixed (just pushed some unrelated fixes in doc to PR) and now the check CI passes on PR branch :-) no clue why.

lorenzwalthert commented 1 year ago

Hi @philipp-baumann. Unfortunately I don’t have any quick hints. The caches you mentioned before could be an issue, yes. Also, I had the same {pkgapi} warning before locally, and I think it had to do with the fact that my GITHUB_TOKEN expired (and there were too many request on the GitHub api when installing packages). So maybe adapting the template to also include such a token of it does not have that already?

Also, if you are keen to contribute to the integration of GitHub lite with {precommit}, all help would be appreciated.

philipp-baumann commented 1 year ago

Thanks Lorenz for the tip. Added now GITHUB_PAT via secrets.GITHUB_TOKEN. Tried again and I could always reproduce and then fix it when deleting the actions cache that failed and pushing a small change again. I brought roxygen2 back because the package tested has no system or other R dependencies required apart base.

I'd be happy to help making the the pre-commit-ci-lite integrated in {precommit}. Really like the auto-fix "bot" for the hooks that support it. Just that I need to get a better understanding of the internals of language support.

As far as I understand there are two pitfalls currently (correct?)

  1. general R language support in pre-commit.ci (lite)
  2. work around build time restrictions (guess both pre-commit.ci and lite version)

Recently I did some more tests using @enchufa2 s {rspm}, which does system-level integration of the RStudio public package manager binary builds (installs into home via apt, dnf etc. and works for major linux OSs) . It integrates really well also with renv (rsmp::renv_enable()) and automatically resolves and installs system dependencies there as well. Integrating that setup on top of a rocker Docker setup would also be quite feasible. What do you think of something along that line?

lorenzwalthert commented 1 year ago

Ok, after thinking a bit more about it, I think I have a clue. When I first set up the template for GitHub Actions, I had similar problems and I think they went away by deactivating the {renv} cache, which is part of the template you get with precommit::use_ci('gha'). Have you tried that template (or why not)? Then, for pre-commit.ci lite support, we can just change that one step that uses the old pre-commit action for the new one.

lorenzwalthert commented 1 year ago

See the template history here: https://github.com/lorenzwalthert/precommit/commits/main/inst/pre-commit-gha.yaml.

I guess the problem with the {renv} cache was that the actual cache is not kept (or some files needed to restore it), but the pre-commit action caches it's own directories that contain a reference to those files. So deactivating the renv cache altogether worked. More elegantly, we would ensure {renv} directories are also cached, so the reference made at pre-commit install would also work.