don't allow Rscript options, minicking --vanilla from user perspective

pre_commit/languages/renv.py

@@ -1,5 +1,6 @@
 import contextlib
 import os
+import shlex
 import shutil
 from typing import Generator
 from typing import Sequence
@@ -26,49 +27,46 @@ def in_env(
     # probably not needed as no env have to be set.
     # Remove this and get_env_patch altogether or keep
     # for consistency with other languages?
-    envdir = prefix.prefix_dir
+    envdir = get_env_dir(prefix, language_version)
     with envcontext(get_env_patch(envdir)):
         yield
 
 
+def get_env_dir(prefix: Prefix, version: str) -> str:
+    return prefix.path(helpers.environment_dir(ENVIRONMENT_DIR, version))
+
+
 def get_env_patch(venv: str) -> PatchesT:
-    # Assumption: location of Rscript on PATH
-    return (('R_PROFILE_USER', os.path.join(venv, 'activate.R')),)
+    return (
+        ('R_PROFILE_USER', os.path.join(venv, 'activate.R')),
+    )
 
 
 def install_environment(
         prefix: Prefix,
         version: str,
         additional_dependencies: Sequence[str],
 ) -> None:
-    directory = helpers.environment_dir(ENVIRONMENT_DIR, version)
-    # installation location currently hardcoded
-    # https://github.com/rstudio/renv/issues/472
-    env_dir = prefix.path(directory)
+    env_dir = get_env_dir(prefix, version)
     with clean_path_on_failure(env_dir):
         os.makedirs(env_dir, exist_ok=True)
         shutil.copy(prefix.path('DESCRIPTION'), env_dir)
-        if os.path.exists(prefix.path('renv.lock')):
-            shutil.copy(prefix.path('renv.lock'), env_dir)
+        shutil.copy(prefix.path('renv.lock'), env_dir)
         cmd_output_b(
             'Rscript', '--vanilla', '-e',
             """
             missing_pkgs <- setdiff(
                 "renv", unname(installed.packages()[, "Package"])
             )
-            options(
+            options(s
                 repos = c(CRAN = "https://cloud.r-project.org"),
                 renv.consent = TRUE
             )
             install.packages(missing_pkgs)
-            if (file.exists('renv.lock')) {
-                renv::restore()
-                renv::activate()
-            } else {
-                renv::init()
-            }
+            renv::activate()
+            renv::restore()
             activate_statement <- paste0(
-              'renv::activate("', file.path(getwd()), '")'
+              'renv::activate("', file.path(getwd()), '"); '
             )
             writeLines(activate_statement, 'activate.R')
 
@@ -78,7 +76,7 @@ def install_environment(
             install.packages("remotes")
             # avoid warning for unset time zone on windows
             Sys.setenv(R_REMOTES_NO_ERRORS_FROM_WARNINGS = "true")
-            remotes::install_local()
+            remotes::install_local(dependencies = FALSE)
             """,
             cwd=env_dir,
         )
@@ -98,22 +96,53 @@ def run_hook(
         color: bool,
 ) -> Tuple[int, bytes]:
     with in_env(hook.prefix, hook.language_version):
-        # TODO handle non-file input (Rscript -e)
-        # TODO forbit --vanilla and --no-init-file as these are
-        # required by renv.
-        # TODO think about how to make sure R_PROFILE_USER can
-        # be respected, i.e. sourced after renv profile.
-        # TODO handle R (not just Rscript)
-        # TODO test all of the above
-
-        # need to prefix path to file as in languages/script.py
-        # as hooks are executed in repo root and unlike in venv
-        # and other languages, installing the hook repo does not
-        # expose these as executables
-        cmd = (*hook.cmd[:1], hook.prefix.path(hook.cmd[-1]))
-        return helpers.run_xargs(hook, cmd, file_args, color=color)
-
-# hooks run in project root, with renv activated from prefix.prefix_dir,
-# uinsg entry: path/to/file which is prefixed to make sure to use the script
-# in the cloned directory. Rscript must be provided in entrypoint for
-# maximum flexibility, but only file backends are supported (-e expr not).
+
+        return helpers.run_xargs(
+            hook, cmd_from_hook(hook), file_args, color=color,
+        )
+
+
+def cmd_from_hook(hook: Hook) -> Tuple[str, ...]:
+    opts = ('--no-save', '--no-restore', '--no-site-file', '--no-environ')
+    entry = shlex.split(hook.entry)
+    entry_validate(entry)
+
+    return (
+        *entry[:1], *opts,
+        *prefix_if_file_entry(entry, hook.prefix),
+        *hook.args,
+    )
+
+
+def entry_validate(entry: Sequence[str]) -> None:
+    """
+    # allowed entries:
+    # Rscript -e expr
+    # Rscript path/to/file
+    # TODO what if expr is x;2? not captured by shlex.split()
+    """
+    if entry[0] != 'Rscript':
+        raise SyntaxError('entry must start with `Rscript`.')
+
+    if entry[1] == '-e':
+        if len(entry) > 3:
+            raise SyntaxError('You can supply at most one expression.')
+    elif len(entry) > 2:
+        raise SyntaxError(
+            ' '.join([
+                'The only valid syntax is `Rscript -e {expr}`',
+                'or `Rscript path/to/hook/script`',
+            ]),
+        )
+
+    None
+
+
+def prefix_if_file_entry(
+    entry: Sequence[str],
+    prefix: Prefix,
+) -> Sequence[str]:
+    if entry[1] == '-e':
+        return(entry[1:])
+    else:
+        return(prefix.path(entry[1]),)

testing/resources/renv_hooks_repo/.pre-commit-hooks.yaml

@@ -1,6 +1,44 @@
+# parsing file
+-   id: parse-file-no-opts-no-args
+    name: Say hi
+    entry: Rscript parse-file-no-opts-no-args.R
+    language: renv
+    files: \.[Rr]$
+-   id: parse-file-no-opts-args
+    name: Say hi
+    entry: Rscript parse-file-no-opts-args.R
+    args: [--no-cache]
+    language: renv
+    files: \.[Rr]$
+## parsing expr
+-   id: parse-expr-no-opts-no-args-1
+    name: Say hi
+    entry: Rscript -e '1+1'
+    language: renv
+    files: \.[Rr]$
+-   id: parse-expr-args-in-entry-2
+    name: Say hi
+    entry: Rscript -e '1+1' -e '3' --no-cache3
+    language: renv
+    files: \.[Rr]$
+# real world
 -   id: hello-world
     name: Say hi
     entry: Rscript hello-world.R
+    args: [blibla]
+    language: renv
+    files: \.[Rr]$
+-   id: hello-world-inline
+    name: Say hi
+    entry: |
+      Rscript -e
+      'stopifnot(
+      packageVersion("rprojroot") == "1.0",
+      packageVersion("gli.clu") == "0.0.0.9000"
+      )
+      cat(commandArgs(trailingOnly = TRUE), "from R!\n", sep = ", ")
+      '
+    args: ['Hi-there']
     language: renv
     files: \.[Rr]$
 -   id: additional-deps

testing/resources/renv_hooks_repo/additional-deps.R

@@ -1,2 +1,2 @@
-suppressPackageStartupMessages(library("here"))
+suppressPackageStartupMessages(library("cachem"))
 cat("OK\n")

testing/resources/renv_hooks_repo/hello-world.R

@@ -1,2 +1,5 @@
-library('rprojroot')
+stopifnot(
+    packageVersion('rprojroot') == '1.0',
+    packageVersion('gli.clu') == '0.0.0.9000'
+)
 cat("Hello, World, from R!\n")

tests/repository_test.py

@@ -18,6 +18,7 @@
 from pre_commit.languages import helpers
 from pre_commit.languages import node
 from pre_commit.languages import python
+from pre_commit.languages import renv
 from pre_commit.languages import ruby
 from pre_commit.languages import rust
 from pre_commit.languages.all import languages
@@ -278,6 +279,79 @@ def test_node_hook_with_npm_userconfig_set(tempdir_factory, store, tmpdir):
         test_run_a_node_hook(tempdir_factory, store)
 
 
+def _test_renv_parsing(
+    tempdir_factory,
+    store,
+    hook_id,
+    expected_hook_expr={},
+    expected_args={},
+):
+    repo_path = 'renv_hooks_repo'
+    path = make_repo(tempdir_factory, repo_path)
+    config = make_config_from_repo(path)
+    hook = _get_hook_no_install(config, store, hook_id)
+    ret = renv.cmd_from_hook(hook)
+    expected_cmd = 'Rscript'
+    expected_opts = (
+        '--no-save', '--no-restore', '--no-site-file', '--no-environ',
+    )
+    expected_path = os.path.join(
+        hook.prefix.prefix_dir, '.'.join([hook_id, 'R']),
+    )
+    expected = (
+        expected_cmd,
+        *expected_opts,
+        *(expected_hook_expr or (expected_path,)),
+        *expected_args,
+    )
+    assert ret == expected
+
+
+def test_renv_parsing_file_no_opts_no_args(tempdir_factory, store):
+    hook_id = 'parse-file-no-opts-no-args'
+    _test_renv_parsing(tempdir_factory, store, hook_id)
+
+
+def test_renv_parsing_file_opts_no_args(tempdir_factory, store):
+
+    with pytest.raises(SyntaxError, match='only valid'):
+        renv.entry_validate(['Rscript', '--no-init', '/path/to/file'])
+
+
+def test_renv_parsing_file_no_opts_args(tempdir_factory, store):
+    hook_id = 'parse-file-no-opts-args'
+    expected_args = ['--no-cache']
+    _test_renv_parsing(
+        tempdir_factory, store, hook_id, expected_args=expected_args,
+    )
+
+
+def test_renv_parsing_expr_no_opts_no_args1(tempdir_factory, store):
+    hook_id = 'parse-expr-no-opts-no-args-1'
+    _test_renv_parsing(
+        tempdir_factory, store, hook_id, expected_hook_expr=('-e', '1+1'),
+    )
+
+
+def test_renv_parsing_expr_no_opts_no_args2(tempdir_factory, store):
+    with pytest.raises(SyntaxError, match='at most one'):
+        renv.entry_validate(['Rscript', '-e', '1+1', '-e', 'letters'])
+
+
+def test_renv_parsing_expr_opts_no_args2(tempdir_factory, store):
+    with pytest.raises(SyntaxError, match='only valid'):
+        renv.entry_validate(
+            [
+                'Rscript', '--vanilla', '-e', '1+1', '-e', 'letters',
+            ],
+        )
+
+
+def test_renv_parsing_expr_args_in_entry2(tempdir_factory, store):
+    with pytest.raises(SyntaxError, match='at most one'):
+        renv.entry_validate(['Rscript', '-e', 'expr1', '--another-arg'])
+
+
 def test_renv_hook(tempdir_factory, store):
     _test_hook_repo(
         tempdir_factory, store, 'renv_hooks_repo',
@@ -286,6 +360,14 @@ def test_renv_hook(tempdir_factory, store):
     )
 
 
+def test_renv_inline_hook(tempdir_factory, store):
+    _test_hook_repo(
+        tempdir_factory, store, 'renv_hooks_repo',
+        'hello-world-inline', ['some-file'],
+        b'Hi-there, some-file, from R!\n',
+    )
+
+
 def test_renv_with_additional_dependencies_hook(tempdir_factory, store):
     _test_hook_repo(
         tempdir_factory, store, 'renv_hooks_repo',
@@ -294,7 +376,7 @@ def test_renv_with_additional_dependencies_hook(tempdir_factory, store):
         config_kwargs={
             'hooks': [{
                 'id': 'additional-deps',
-                'additional_dependencies': ['here'],
+                'additional_dependencies': ['cachem@1.0.4'],
             }],
         },
     )
@@ -307,7 +389,7 @@ def test_run_a_ruby_hook(tempdir_factory, store):
     )
 
 
-@xfailif_windows  # pragma: win32 no cover
+@ xfailif_windows  # pragma: win32 no cover
 def test_run_versioned_ruby_hook(tempdir_factory, store):
     _test_hook_repo(
         tempdir_factory, store, 'ruby_versioned_hooks_repo',
@@ -317,11 +399,13 @@ def test_run_versioned_ruby_hook(tempdir_factory, store):
     )
 
 
-@xfailif_windows  # pragma: win32 no cover
+@ xfailif_windows  # pragma: win32 no cover
 def test_run_ruby_hook_with_disable_shared_gems(
         tempdir_factory,
         store,
         tmpdir,
+
+
 ):
     """Make sure a Gemfile in the project doesn't interfere."""
     tmpdir.join('Gemfile').write('gem "lol_hai"')