You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The implementation in #1799 relies on a globally installed version of the {renv} R package (with 0 dependencies). When the version of this installed package is different from what is provided in renv.lock (which is managed by the hook author), a warning like the following is displayed when hooks are ran:
renv 0.13.0 was loaded from project library, but this project is configured to use renv 0.12.5.
Use `renv::record("renv@0.13.0")` to record renv 0.13.0 in the lockfile.
Use `renv::restore(packages = "renv")` to install renv 0.12.5 into the project library.
This is very misleading since these commands had to be ran in env_dir of the hook, not in the directory from which the hook is invoked and if the user were to run the above command, it would in the worst case change the virtual environment of the project from which the hooks are invoked - and in no case fix the warning.
The solution to this is IMO, as mentioned in #1799 (comment), to require the hook authors to provide the activation script renv/activate.R in the hook repo in addition to renv.lock, as recommended for collaboration per the docs of {renv} (.Rprofile can be omitted since all it contains is a call to renv/activate.R). This will boostrap the right version of {renv} installation into the virtual environment used for the hook. This approach fixes a few other problematic aspects of the current implementation in addition to the above:
{renv} would no longer be installed into the global package library. We could go from a 99% insolation to 100% isolation from the global library.
{renv} is not required in the global package library, i.e. the hooks still work if it was for some reason uninstalled or not available after a major R version update (where the user starts off with an empty library).
If you agree, I can make a PR to try to implement this. For local hooks, we need to make sure to populate the directory renv/ in env_dir with activate.R in analogy to what the hook author would provide in his repo. Currently, pre_commit.store.Store.make_local() can only put files in the root of env_dir so this would require making the function slightly more general and add "renv/activate.R" to pre_commit.store.Store.LOCAL_RESOURCES as well as creating empty_template_activate.R in pre_commit/resources. I believe allowing to place files in sub directories of env_dir with pre_commit.Store.make_local() can be achieved with just a few tweaks to the function (lorenzwalthert@4218765).
Sorry for the hassle, I should have done more testing and indicated concerns in #1799 after I converted from draft and all checks were passing.
The implementation in #1799 relies on a globally installed version of the
{renv}R package (with 0 dependencies). When the version of this installed package is different from what is provided inrenv.lock(which is managed by the hook author), a warning like the following is displayed when hooks are ran:This is very misleading since these commands had to be ran in
env_dirof the hook, not in the directory from which the hook is invoked and if the user were to run the above command, it would in the worst case change the virtual environment of the project from which the hooks are invoked - and in no case fix the warning.The solution to this is IMO, as mentioned in #1799 (comment), to require the hook authors to provide the activation script
renv/activate.Rin the hook repo in addition torenv.lock, as recommended for collaboration per the docs of{renv}(.Rprofilecan be omitted since all it contains is a call torenv/activate.R). This will boostrap the right version of{renv}installation into the virtual environment used for the hook. This approach fixes a few other problematic aspects of the current implementation in addition to the above:{renv}would no longer be installed into the global package library. We could go from a 99% insolation to 100% isolation from the global library.{renv}is not required in the global package library, i.e. the hooks still work if it was for some reason uninstalled or not available after a major R version update (where the user starts off with an empty library).If you agree, I can make a PR to try to implement this. For local hooks, we need to make sure to populate the directory
renv/inenv_dirwithactivate.Rin analogy to what the hook author would provide in his repo. Currently,pre_commit.store.Store.make_local()can only put files in the root ofenv_dirso this would require making the function slightly more general and add"renv/activate.R"topre_commit.store.Store.LOCAL_RESOURCESas well as creatingempty_template_activate.Rinpre_commit/resources. I believe allowing to place files in sub directories ofenv_dirwithpre_commit.Store.make_local()can be achieved with just a few tweaks to the function (lorenzwalthert@4218765).Sorry for the hassle, I should have done more testing and indicated concerns in #1799 after I converted from draft and all checks were passing.