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

Catch up with pre-commit.ci lite #450

Open
lorenzwalthert opened this issue Nov 15, 2022 · 6 comments
Open

Catch up with pre-commit.ci lite #450

lorenzwalthert opened this issue Nov 15, 2022 · 6 comments

Comments

@lorenzwalthert
Copy link
Owner

pre-commit-ci/issues#13

@philipp-baumann
Copy link

philipp-baumann commented Dec 24, 2022

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
Copy link

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
Copy link
Owner Author

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
Copy link

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
Copy link
Owner Author

lorenzwalthert commented Dec 24, 2022

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
Copy link
Owner Author

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.

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

No branches or pull requests

2 participants