From 9c88fa782557ac96f3d4c61d7646811e30c365cd Mon Sep 17 00:00:00 2001 From: Lorenz Date: Tue, 23 Feb 2021 14:49:29 +0100 Subject: [PATCH] don't allow Rscript options, minicking --vanilla from user perspective --- pre_commit/languages/renv.py | 103 +++++++++++------- .../renv_hooks_repo/.pre-commit-hooks.yaml | 38 +++++++ .../renv_hooks_repo/additional-deps.R | 2 +- .../resources/renv_hooks_repo/hello-world.R | 5 +- tests/repository_test.py | 90 ++++++++++++++- 5 files changed, 196 insertions(+), 42 deletions(-) diff --git a/pre_commit/languages/renv.py b/pre_commit/languages/renv.py index 9cc3ce266..c37006d8a 100644 --- a/pre_commit/languages/renv.py +++ b/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,14 +27,19 @@ 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( @@ -41,34 +47,26 @@ def install_environment( 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]),) diff --git a/testing/resources/renv_hooks_repo/.pre-commit-hooks.yaml b/testing/resources/renv_hooks_repo/.pre-commit-hooks.yaml index af94659e7..6988552ca 100644 --- a/testing/resources/renv_hooks_repo/.pre-commit-hooks.yaml +++ b/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 diff --git a/testing/resources/renv_hooks_repo/additional-deps.R b/testing/resources/renv_hooks_repo/additional-deps.R index 360712c5a..bc145951b 100755 --- a/testing/resources/renv_hooks_repo/additional-deps.R +++ b/testing/resources/renv_hooks_repo/additional-deps.R @@ -1,2 +1,2 @@ -suppressPackageStartupMessages(library("here")) +suppressPackageStartupMessages(library("cachem")) cat("OK\n") diff --git a/testing/resources/renv_hooks_repo/hello-world.R b/testing/resources/renv_hooks_repo/hello-world.R index 32810c751..bf8d92f42 100755 --- a/testing/resources/renv_hooks_repo/hello-world.R +++ b/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") diff --git a/tests/repository_test.py b/tests/repository_test.py index 617fc7ab1..0f5d7e65e 100644 --- a/tests/repository_test.py +++ b/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"')