Repository: pre-commit/pre-commit

Issue:
RScript location not always correct
This rarely comes up, because most people (in my experience) don't explicitly set R_HOME in regular terminals, but do have `RScript` in the path. If you use the RStudio-supplied Terminal, however, they do explicitly set R_HOME, which then causes the hook installs to break on some platforms (e.g., AWS Ubuntu is where I encountered the problem).

While some distributions add symlinks, the default install location for `RScript` is `$R_HOME/bin/Rscript`:
https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installation

I believe it can be fixed in just one spot:

https://github.com/pre-commit/pre-commit/blob/354b900f15e88a06ce8493e0316c288c44777017/pre_commit/languages/r.py#L62

Just adding `bin` to that path join should solve the issue.

Discussion:
lorenzwalthert: This currently does not work because we also need to account for the fact that `R_HOME` might not be set (as it seems in the test suite). In that case,`bin/Rscript` Is returned. 

To be true to the name of the function, maybe we should look up if `Rscript` is on the `PATH` and return that full path or - if `Rscript` is not on the `PATH` - return `$R_HOME/bin/Rscript`, and error if `$R_HOME` is not set. 
jeff-m-sullivan: Of course! Sorry I missed that. I'll put together something more thorough along those lines.
lorenzwalthert: Cool. Let me know if you need help. Also, we should rename the function `_rscript_exec` to `_path_rscript_exec()` as it now always returns a path to the executable and it would be great if you could also test to `tests/languages/r_test.py`. You can ensure the same env as in the execution is present and set R home in a similar way:

Then, we should probably add a test along these lines.
```python
from pre_commit.languages.r import in_env, _path_rscript_exec
from pre_commit.envcontext import envcontext

@pytest.mark.parametrize(('r_home_env', 'rscript_path_expected') [("/usr/bin/local", "/user/bin/local/Rscript"), (UNSET, 'location is os specific: get it dynamically)])
def test_path_rscript_exec(r_home_env, rscript_path_expected):
    with in_env(hook.prefix, hook.language_version):
        with envcontext(("R_HOME", r_home_env)): 
            assert _path_rscript_exec() == rscript_path_expected
```
 If it gives an error or takes too long, I can figure that out later.
lorenzwalthert: Actually we should not really need `with in_env…`

Patch:
diff --git a/pre_commit/languages/r.py b/pre_commit/languages/r.py
index 9b32b2d..c736b38 100644
--- a/pre_commit/languages/r.py
+++ b/pre_commit/languages/r.py
@@ -59,7 +59,11 @@ def _prefix_if_non_local_file_entry(
 
 
 def _rscript_exec() -> str:
-    return os.path.join(os.getenv('R_HOME', ''), 'Rscript')
+    r_home = os.environ.get('R_HOME')
+    if r_home is None:
+        return 'Rscript'
+    else:
+        return os.path.join(r_home, 'bin', 'Rscript')
 
 
 def _entry_validate(entry: Sequence[str]) -> None:


Test patch:
diff --git a/tests/languages/r_test.py b/tests/languages/r_test.py
index bc302a7..5bc63b2 100644
--- a/tests/languages/r_test.py
+++ b/tests/languages/r_test.py
@@ -4,6 +4,7 @@ import os.path
 
 import pytest
 
+from pre_commit import envcontext
 from pre_commit.languages import r
 from testing.fixtures import make_config_from_repo
 from testing.fixtures import make_repo
@@ -129,3 +130,14 @@ def test_r_parsing_file_local(tempdir_factory, store):
         config=config,
         expect_path_prefix=False,
     )
+
+
+def test_rscript_exec_relative_to_r_home():
+    expected = os.path.join('r_home_dir', 'bin', 'Rscript')
+    with envcontext.envcontext((('R_HOME', 'r_home_dir'),)):
+        assert r._rscript_exec() == expected
+
+
+def test_path_rscript_exec_no_r_home_set():
+    with envcontext.envcontext((('R_HOME', envcontext.UNSET),)):
+        assert r._rscript_exec() == 'Rscript'