Skip to content

don't use system for ruby/node if it is a shim exe #1668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
asottile merged 1 commit into from
Oct 29, 2020
Merged

don't use system for ruby/node if it is a shim exe #1668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions pre_commit/languages/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import multiprocessing
import os
import random
import re
from typing import Any
from typing import List
from typing import Optional
Expand All @@ -10,6 +11,7 @@
from typing import TYPE_CHECKING

import pre_commit.constants as C
from pre_commit import parse_shebang
from pre_commit.hook import Hook
from pre_commit.prefix import Prefix
from pre_commit.util import cmd_output_b
Expand All @@ -20,6 +22,25 @@

FIXED_RANDOM_SEED = 1542676187

SHIMS_RE = re.compile(r'[/\\]shims[/\\]')


def exe_exists(exe: str) -> bool:
found = parse_shebang.find_executable(exe)
if found is None: # exe exists
return False

homedir = os.path.expanduser('~')
try:
common: Optional[str] = os.path.commonpath((found, homedir))
except ValueError: # on windows, different drives raises ValueError
common = None

return (
not SHIMS_RE.search(found) and # it is not in a /shims/ directory
common != homedir # it is not in the home directory
)


def run_setup_cmd(prefix: Prefix, cmd: Tuple[str, ...]) -> None:
cmd_output_b(*cmd, cwd=prefix.prefix_dir)
Expand Down
3 changes: 1 addition & 2 deletions pre_commit/languages/node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from typing import Tuple

import pre_commit.constants as C
from pre_commit import parse_shebang
from pre_commit.envcontext import envcontext
from pre_commit.envcontext import PatchesT
from pre_commit.envcontext import UNSET
Expand All @@ -31,7 +30,7 @@ def get_default_version() -> str:
return C.DEFAULT
# if node is already installed, we can save a bunch of setup time by
# using the installed version
elif all(parse_shebang.find_executable(exe) for exe in ('node', 'npm')):
elif all(helpers.exe_exists(exe) for exe in ('node', 'npm')):
return 'system'
else:
return C.DEFAULT
Expand Down
3 changes: 1 addition & 2 deletions pre_commit/languages/ruby.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from typing import Tuple

import pre_commit.constants as C
from pre_commit import parse_shebang
from pre_commit.envcontext import envcontext
from pre_commit.envcontext import PatchesT
from pre_commit.envcontext import UNSET
Expand All @@ -26,7 +25,7 @@

@functools.lru_cache(maxsize=1)
def get_default_version() -> str:
if all(parse_shebang.find_executable(exe) for exe in ('ruby', 'gem')):
if all(helpers.exe_exists(exe) for exe in ('ruby', 'gem')):
return 'system'
else:
return C.DEFAULT
Expand Down
45 changes: 44 additions & 1 deletion tests/languages/helpers_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,60 @@
import multiprocessing
import os
import os.path
import sys
from unittest import mock

import pytest

import pre_commit.constants as C
from pre_commit import parse_shebang
from pre_commit.languages import helpers
from pre_commit.prefix import Prefix
from pre_commit.util import CalledProcessError
from testing.auto_namedtuple import auto_namedtuple


@pytest.fixture
def find_exe_mck():
with mock.patch.object(parse_shebang, 'find_executable') as mck:
yield mck


@pytest.fixture
def homedir_mck():
def fake_expanduser(pth):
assert pth == '~'
return os.path.normpath('/home/me')

with mock.patch.object(os.path, 'expanduser', fake_expanduser):
yield


def test_exe_exists_does_not_exist(find_exe_mck, homedir_mck):
find_exe_mck.return_value = None
assert helpers.exe_exists('ruby') is False


def test_exe_exists_exists(find_exe_mck, homedir_mck):
find_exe_mck.return_value = os.path.normpath('/usr/bin/ruby')
assert helpers.exe_exists('ruby') is True


def test_exe_exists_false_if_shim(find_exe_mck, homedir_mck):
find_exe_mck.return_value = os.path.normpath('/foo/shims/ruby')
assert helpers.exe_exists('ruby') is False


def test_exe_exists_false_if_homedir(find_exe_mck, homedir_mck):
find_exe_mck.return_value = os.path.normpath('/home/me/somedir/ruby')
assert helpers.exe_exists('ruby') is False


def test_exe_exists_commonpath_raises_ValueError(find_exe_mck, homedir_mck):
find_exe_mck.return_value = os.path.normpath('/usr/bin/ruby')
with mock.patch.object(os.path, 'commonpath', side_effect=ValueError):
assert helpers.exe_exists('ruby') is True


def test_basic_get_default_version():
assert helpers.basic_get_default_version() == C.DEFAULT

Expand Down