precommit icon indicating copy to clipboard operation
precommit copied to clipboard

Broken activate.R in renv-default for precommit compatibility with projects using renv

Open smingerson opened this issue 3 years ago • 17 comments
trafficstars

Back again! Thank you for improving the support for this use case. I can't get precommit hooks to complete in new project (which uses renv). This is using renv 0.15.2.9 and precommit 0.2.2.9010, the latest on Github. Notably, the hooks I can't get to complete are those which depend on particular files existing, such as the styler hook.

The errors are in the vein of "Cannot find file _targets.Rmd". Also, the backtrace was 500+ lines long, with essentially the same calls repeated. This made me suspicious about working directory changes, so I opened the precommit renv-default/activate.R and found:

# Maybe this is slightly wrong, I had done much editing of the offending file.
suppressWarnings({old <- setwd("C:/Users/<user>/.cache/pre-commit/repos7295wzc/renv-default"); source("renv/activate.R"); setwd(old); renv::load("C:/Users/<user>/.cache/pre-commit/repos7295wzc/renv-default");})

I believe the issue is that source("renv/activate.R)" and renv::load(path) accomplish the same task. I changed my renv-default/activate.R to contain:

suppressWarnings({old <- setwd("C:/Users/<user>/.cache/pre-commit/repos7295wzc/renv-default"); renv::load("C:/Users/<user>/.cache/pre-commit/repos7295wzc/renv-default");setwd(old)})

Precommit hooks such as styling then began working as expected.

smingerson avatar Feb 14 '22 17:02 smingerson

Thanks @smingerson, I will look into that. Meanwhile, you can check out what Kevin wrote about load() Vs. source(„renv/activate“) in https://github.com/rstudio/renv/issues/900:

That seems strange to me -- calling source("renv/activate.R") should be sufficient, since the job of the autoloader is to call renv::load() anyhow.

From that perspective, we should probably rather drop the renv::load(), not source("renv/activate.R"). Can you tell me if that works too?

Also, did you update {renv} in your global R Library too or just the version used kn tust project?

lorenzwalthert avatar Feb 14 '22 21:02 lorenzwalthert

renv-default/renv/activate.R requests 0.15.0-13. My project uses 0.15.2.9 (I think it was 0.14.0, I upgraded after running into problem), and my global library is at 0.14.0. I'd guess from the error that the version of renv is not relevant.

Using source("renv/activate.R") does not work -- I am back to a similar error message, without the ridiculous recursion.


During startup - Warning messages:
1: Setting LC_COLLATE=en_US.UTF-8 failed 
2: Setting LC_CTYPE=en_US.UTF-8 failed 
3: Setting LC_MONETARY=en_US.UTF-8 failed 
4: Setting LC_TIME=en_US.UTF-8 failed 
Error in normalizePath(path.expand(path), winslash, mustWork) : 
  path[1]="README.md": The system cannot find the file specified
Calls: <Anonymous> -> sort -> normalizePath
Execution halted
readme-rmd-rendered......................................................Passed
parsable-R...........................................(no files to check)Skipped
no-browser-statement.................................(no files to check)Skippedcheck for added large files..............................................Passedfix end of files.........................................................Passed
Don't commit common R artifacts......................(no files to check)Skipped

smingerson avatar Feb 15 '22 13:02 smingerson

Can you @smingerson try the latest main branch? I.e. in your .pre-commit-config.yaml, use rev: 45e412b for the hook repo instead of 0.2.2.9010?

The problem was probably with the file I provided in renv/activate.R, it was based on the GitHub version of {renv} instead of the CRAN version.

lorenzwalthert avatar Feb 15 '22 15:02 lorenzwalthert

Switching to rev: 45e412b in my personal project .pre-commit-config.yaml leads to similar recursive calls with styling error .Rprofile Error: cannot open file '.Rprofile': No such file or directoryExecution halted (I was committing changes to .Rprofile.

The activate.R contents don't look different based on the call in the error message.

 suppressWarnings({
          old <- setwd("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
          source("renv/activate.R")
          setwd(old)
          renv::load("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
      })

smingerson avatar Feb 15 '22 15:02 smingerson

Did you re-initialize the project? That activate.R is not supposed to be changed, whats supposed to change is renv/activate.R. Can you confirm this has the right version in line 5?

lorenzwalthert avatar Feb 15 '22 15:02 lorenzwalthert

If the problem still occurs, we might indeed have to change activate.R, but that won't be here but in the upstream pre-commit framework.

lorenzwalthert avatar Feb 15 '22 15:02 lorenzwalthert

Precommit generated a new environment if that is what you mean by re-initialize. From the precommit project, renv-default/renv/activate.R requests version 0.15.2.

smingerson avatar Feb 15 '22 15:02 smingerson

Precommit generated a new environment if that is what you mean by re-initialize.

like run $ pre-commit clean and $ pre-commit install in that git repo.

lorenzwalthert avatar Feb 15 '22 15:02 lorenzwalthert

Thank you for clarifying. I ran that in my project git repo, then tried to commit a file. No luck, getting the same type of errors.

smingerson avatar Feb 15 '22 15:02 smingerson

Thanks for your persistence. Can you try again (with $ pre-commit clean and $ pre-commit install) with setting your activate.R to (commenting out the lines from your previous example).

 suppressWarnings({
          old <- setwd("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
          source("renv/activate.R")
          setwd(old)
          # renv::load("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
      })

And tell me if it works as well as if this works:

 suppressWarnings({
          # old <- setwd("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
          # source("renv/activate.R")
          # setwd(old)
          renv::load("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
      })

As said earlier, I'd prefer the first solution because that's what Kevin suggests should work. Also, for completeness, what R version are you using? And I see you are using Windows, so maybe the problem does not generalize to other platforms. If it's a public repo, you can link it here and I might try on my macOS.

lorenzwalthert avatar Feb 15 '22 15:02 lorenzwalthert

You're welcome! Well worth the time to ease use of such a great tool.

I'm on 4.1.1.

Unfortunately I cannot share it, but it is essentially a bare RStudio project. I can tinker with a new project and try to reproduce on Linux sometime this week. When I do it on Linux I will record the exact steps I take.

# This fails -- In an earlier run I had added `cat(getwd())` and it showed it never switched back from renv-default
 suppressWarnings({
          old <- setwd("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
          source("renv/activate.R")
          setwd(old)
          # renv::load("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
      })
# This works
 suppressWarnings({
          # old <- setwd("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
          # source("renv/activate.R")
          # setwd(old)
          renv::load("C:/Users/<user>/.cache/pre-commit/repozn2_1oz_/renv-default")
      })

smingerson avatar Feb 15 '22 16:02 smingerson

Unfortunately I cannot share it, but it is essentially a bare RStudio project. I can tinker with a new project and try to reproduce on Linux sometime this week

That would be great. Also, I suggest to omit the suppressWarnings() (this we could also do upstream regardless, and re-throw a warning) for more informative output. I still don't understand why {renv} can't activate the project or change the working directory, which would be useful information.

From above, I missed:

I'd guess from the error that the version of renv is not relevant.

I think it is, because of https://github.com/rstudio/renv/issues/900. I suggest you upgrade your global {renv} first before doing more experimentation. Maybe all we'll need a minimal version requirement in the activate.R.

lorenzwalthert avatar Feb 15 '22 20:02 lorenzwalthert

I think it is, because of https://github.com/rstudio/renv/issues/900. I suggest you upgrade your global {renv} first before doing more experimentation. Maybe all we'll need a minimal version requirement in the activate.R.

I upgraded my global to 0.15.2 and still having the same issues.

Didn't get to trying on Linux yet, hopefully will in next few days.

smingerson avatar Feb 21 '22 13:02 smingerson

Ok, thanks. What the pre-commit version you are using? Because it was also fixed there in release v2.17.0 to cover the case when you have an out-of-data {renv} version and a newer pre-commit version.

lorenzwalthert avatar Feb 23 '22 11:02 lorenzwalthert

I'm at 2.14.0. Should I update it?

smingerson avatar Feb 23 '22 15:02 smingerson

I don’t expect it to solve the issue, but let’s upgrade and rule this out.

lorenzwalthert avatar Feb 23 '22 19:02 lorenzwalthert

@smingerson this is an important issue. Any progress on a sharable reprex? Maybe even one that can run on GitHub Actions?

lorenzwalthert avatar May 03 '22 09:05 lorenzwalthert