From f1295e0fb85a7e48a021b91dbb965d0c68ce989e Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Wed, 13 Jul 2022 17:16:13 +0100 Subject: [PATCH] Apply suggestions from code review This commit applies suggestions from the code review in https://github.com/hypothesis/pip-sync-faster/pull/6 --- .cookiecutter/includes/HACKING.md | 49 ++++++++ .cookiecutter/includes/README.md | 21 ++++ .cookiecutter/includes/tox/deps | 1 + HACKING.md | 49 ++++++++ README.md | 25 +++- pyproject.toml | 3 + setup.cfg | 2 +- src/pip_sync_faster/__main__.py | 5 + src/pip_sync_faster/cli.py | 28 +++++ src/pip_sync_faster/main.py | 71 ------------ src/pip_sync_faster/sync.py | 50 ++++++++ tests/functional/pip_sync_faster_test.py | 2 - tests/unit/pip_sync_faster/cli_test.py | 37 ++++++ tests/unit/pip_sync_faster/main_test.py | 3 - tests/unit/pip_sync_faster/sync_test.py | 139 +++++++++++++++++++++++ tox.ini | 1 + 16 files changed, 407 insertions(+), 79 deletions(-) create mode 100644 .cookiecutter/includes/HACKING.md create mode 100644 .cookiecutter/includes/tox/deps create mode 100644 src/pip_sync_faster/__main__.py create mode 100644 src/pip_sync_faster/cli.py delete mode 100644 src/pip_sync_faster/main.py create mode 100644 src/pip_sync_faster/sync.py delete mode 100644 tests/functional/pip_sync_faster_test.py create mode 100644 tests/unit/pip_sync_faster/cli_test.py delete mode 100644 tests/unit/pip_sync_faster/main_test.py create mode 100644 tests/unit/pip_sync_faster/sync_test.py diff --git a/.cookiecutter/includes/HACKING.md b/.cookiecutter/includes/HACKING.md new file mode 100644 index 0000000..4a64e44 --- /dev/null +++ b/.cookiecutter/includes/HACKING.md @@ -0,0 +1,49 @@ + +Testing Manually +---------------- + +Normally if you wanted to test a command manually in dev you'd do so through +tox, for example: + +```terminal +$ tox -qe dev --run-command 'pip-sync-faster --help' +usage: pip-sync-faster [-h] [-v] + +options: + -h, --help show this help message and exit + -v, --version +``` + +But there's a problem with running `pip-sync-faster` commands in this way: a +command like `tox -e dev --run-command 'pip-sync-faster requirements.txt'` will +run `pip-sync requirements.txt` and `pip-sync` will sync the +current virtualenv (`.tox/dev/`) with the `requirements.txt` file. Everything +in `requirements.txt` will get installed into `.tox/dev/`, which you probably +don't want. Even worse everything _not_ in `requirements.txt` will get +_removed_ from `.tox/dev/` including `pip-sync-faster` itself! + +To avoid this problem run `pip-sync-faster` in a temporary virtualenv instead. +This installs the contents of `requirements.txt` into the temporary venv so +your `.tox/dev/` env doesn't get messed up. And it does not install +`pip-sync-faster` into the temporary venv so there's no issue with `pip-sync` +uninstalling `pip-sync-faster`: + +```terminal +# Make a temporary directory. +tempdir=$(mktemp -d) + +# Create a virtualenv in the temporary directory. +python3 -m venv $tempdir + +# Activate the virtualenv. +source $tempdir/bin/activate + +# Install pip-tools in the virtualenv (pip-sync-faster needs pip-tools). +pip install pip-tools + +# Call pip-sync-faster to install a requirements file into the temporary virtualenv. +PYTHONPATH=src python3 -m pip_sync_faster /path/to/requirements.txt + +# When you're done testing deactivate the temporary virtualenv. +deactivate +``` diff --git a/.cookiecutter/includes/README.md b/.cookiecutter/includes/README.md index 9753d0b..e25b871 100644 --- a/.cookiecutter/includes/README.md +++ b/.cookiecutter/includes/README.md @@ -27,6 +27,27 @@ If any of the given requirements files doesn't have a matching cached hash then pip-sync-faster calls pip-sync forwarding all command line arguments and options. +## You need to add `pip-sync-faster` to your requirements file + +A command like `pip-sync-faster requirements.txt` will call +`pip-sync requirements.txt` which will uninstall anything not in +`requirements.txt` from the active venv, including `pip-sync-faster` itself! + +You can add `pip-sync-faster` to `requirements.txt` so that it doesn't get +uninstalled. + +### Running `pip-sync-faster` directly instead + +Alternatively as long as `pip-tools` is installed in the active venv you can +run `pip-sync-faster` directly with a command like: + +```bash +PYTHONPATH=/path/to/pip-sync-faster/src python3 -m pip_sync_faster requirements.txt +``` + +This doesn't rely on `pip-sync-faster` being installed so there's no issue with +`pip-sync` uninstalling it. + ## pip-sync-faster doesn't sync modified virtualenvs If you modify your requirements files pip-sync-faster will notice the change diff --git a/.cookiecutter/includes/tox/deps b/.cookiecutter/includes/tox/deps new file mode 100644 index 0000000..b70bb7a --- /dev/null +++ b/.cookiecutter/includes/tox/deps @@ -0,0 +1 @@ +lint,tests: pytest-mock diff --git a/HACKING.md b/HACKING.md index e34cb34..20fe4f2 100644 --- a/HACKING.md +++ b/HACKING.md @@ -100,3 +100,52 @@ To change the project's formatting, linting and test dependencies: ``` 3. Commit everything to git and send a pull request + +Testing Manually +---------------- + +Normally if you wanted to test a command manually in dev you'd do so through +tox, for example: + +```terminal +$ tox -qe dev --run-command 'pip-sync-faster --help' +usage: pip-sync-faster [-h] [-v] + +options: + -h, --help show this help message and exit + -v, --version +``` + +But there's a problem with running `pip-sync-faster` commands in this way: a +command like `tox -e dev --run-command 'pip-sync-faster requirements.txt'` will +run `pip-sync requirements.txt` as a subprocess and `pip-sync` will sync the +current virtualenv (`.tox/dev/`) with the `requirements.txt` file. Everything +in `requirements.txt` will get installed into `.tox/dev/`, which you probably +don't want. Even worse everything _not_ in `requirements.txt` will get +_removed_ from `.tox/dev/` including `pip-sync-faster` itself! + +To avoid this problem run `pip-sync-faster` in a temporary virtualenv instead. +This installs the contents of `requirements.txt` into the temporary venv so +your `.tox/dev/` env doesn't get messed up. And it does not install +`pip-sync-faster` into the temporary venv so there's no issue with `pip-sync` +uninstalling `pip-sync-faster`: + +```terminal +# Make a temporary directory. +tempdir=$(mktemp -d) + +# Create a virtualenv in the temporary directory. +python3 -m venv $tempdir + +# Activate the virtualenv. +source $tempdir/bin/activate + +# Install pip-tools in the virtualenv (pip-sync-faster needs pip-tools). +pip install pip-tools + +# Call pip-sync-faster to install a requirements file into the temporary virtualenv. +PYTHONPATH=src python3 -m pip_sync_faster /path/to/requirements.txt + +# When you're done testing deactivate the temporary virtualenv. +deactivate +``` diff --git a/README.md b/README.md index 9d753bd..5206833 100644 --- a/README.md +++ b/README.md @@ -35,13 +35,34 @@ user 0m0.029s sys 0m0.008s ``` -pip-sync-faster does this by saving hashes of the given requirements files in a +`pip-sync-faster` does this by saving hashes of the given requirements files in a JSON file within the virtualenv and not calling pip-sync if the hashes haven't changed. If any of the given requirements files doesn't have a matching cached hash then pip-sync-faster calls pip-sync forwarding all command line arguments and options. +## You need to add `pip-sync-faster` to your requirements file + +A command like `pip-sync-faster requirements.txt` will call +`pip-sync requirements.txt` which will uninstall anything not in +`requirements.txt` from the active venv, including `pip-sync-faster` itself! + +You can add `pip-sync-faster` to `requirements.txt` so that it doesn't get +uninstalled. + +### Running `pip-sync-faster` directly instead + +Alternatively as long as `pip-tools` is installed in the active venv you can +run `pip-sync-faster` directly with a command like: + +```bash +PYTHONPATH=/path/to/pip-sync-faster/src python3 -m pip_sync_faster requirements.txt +``` + +This doesn't rely on `pip-sync-faster` being installed so there's no issue with +`pip-sync` uninstalling it. + ## pip-sync-faster doesn't sync modified virtualenvs If you modify your requirements files pip-sync-faster will notice the change @@ -54,4 +75,4 @@ Calling pip-sync directly in this case would re-sync your virtualenv with your requirements files, but calling pip-sync-faster won't. If you can live with this limitation then you can use pip-sync-faster and save -yourself a few hundred milliseconds. If not you should just use pip-sync. +yourself a few hundred milliseconds. If not you should just use pip-sync. diff --git a/pyproject.toml b/pyproject.toml index 1462022..89a4b6c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -43,6 +43,9 @@ ignore = [ branch = true parallel = true source = ["pip_sync_faster", "tests/unit"] +omit = [ + "*/pip_sync_faster/__main__.py", +] [tool.coverage.paths] source = ["src", ".tox/*tests/lib/python*/site-packages"] diff --git a/setup.cfg b/setup.cfg index c3982b2..01dc9d9 100644 --- a/setup.cfg +++ b/setup.cfg @@ -25,7 +25,7 @@ where = src [options.entry_points] console_scripts = - pip-sync-faster = pip_sync_faster.main:entry_point + pip-sync-faster = pip_sync_faster.cli:cli [pycodestyle] ignore = diff --git a/src/pip_sync_faster/__main__.py b/src/pip_sync_faster/__main__.py new file mode 100644 index 0000000..ff96285 --- /dev/null +++ b/src/pip_sync_faster/__main__.py @@ -0,0 +1,5 @@ +import sys + +from pip_sync_faster.cli import cli + +sys.exit(cli()) diff --git a/src/pip_sync_faster/cli.py b/src/pip_sync_faster/cli.py new file mode 100644 index 0000000..769870d --- /dev/null +++ b/src/pip_sync_faster/cli.py @@ -0,0 +1,28 @@ +from argparse import ArgumentParser +from importlib.metadata import version +from subprocess import CalledProcessError + +from pip_sync_faster.sync import sync + + +def cli(_argv=None): # pylint:disable=inconsistent-return-statements + parser = ArgumentParser( + description="Synchronize the active venv with requirements.txt files." + ) + parser.add_argument( + "--version", action="store_true", help="show the version and exit" + ) + parser.add_argument( + "src_files", nargs="*", help="the requirements.txt files to synchronize" + ) + + args = parser.parse_known_args(_argv) + + if args[0].version: + print(f"pip-sync-faster, version {version('pip-sync-faster')}") + return + + try: + sync(args[0].src_files) + except CalledProcessError as err: + return err.returncode diff --git a/src/pip_sync_faster/main.py b/src/pip_sync_faster/main.py deleted file mode 100644 index 88b9ade..0000000 --- a/src/pip_sync_faster/main.py +++ /dev/null @@ -1,71 +0,0 @@ -import hashlib -import json -import os -import sys -from argparse import ArgumentParser -from functools import cached_property -from importlib.metadata import version -from pathlib import Path -from subprocess import CalledProcessError, run - - -class SrcFile: - def __init__(self, path): - self.path = path - - @cached_property - def abspath(self): - return os.path.abspath(self.path) - - @cached_property - def contents_hash(self): - hash_ = hashlib.sha512() - with open(self.abspath, "rb") as file: - hash_.update(file.read()) - return hash_.hexdigest() - - -def pip_sync_maybe(src_files, args): - cached_hashes_path = Path(os.environ["VIRTUAL_ENV"]) / "pip_sync_faster.json" - - try: - with open(cached_hashes_path, "r", encoding="utf8") as cached_hashes_file: - cached_hashes = json.load(cached_hashes_file) - except FileNotFoundError: - cached_hashes = {} - - src_files = [SrcFile(src_file) for src_file in src_files] - - for src_file in src_files: - if src_file.contents_hash != cached_hashes.get(src_file.abspath): - break - else: - # All of the source files already had matching hashes in the cache. - return - - # One or more of the source files was either missing from the cache or had - # a non-matching hash in the cache. - try: - run(["pip-sync"] + args, check=True) - except CalledProcessError as err: - sys.exit(err.returncode) - else: - # pip-sync succeeded so update the cache. - for src_file in src_files: - cached_hashes[src_file.abspath] = src_file.contents_hash - - with open(cached_hashes_path, "w", encoding="utf8") as cached_hashes_file: - json.dump(cached_hashes, cached_hashes_file) - - -def entry_point(): - parser = ArgumentParser() - parser.add_argument("--version", action="store_true") - parser.add_argument("src_files", nargs="*") - - args = parser.parse_known_args() - - if args[0].version: - print(f"pip-sync-faster, version {version('pip-sync-faster')}") - - pip_sync_maybe(args[0].src_files, sys.argv[1:]) diff --git a/src/pip_sync_faster/sync.py b/src/pip_sync_faster/sync.py new file mode 100644 index 0000000..1212bb9 --- /dev/null +++ b/src/pip_sync_faster/sync.py @@ -0,0 +1,50 @@ +import hashlib +import json +import sys +from os import environ +from os.path import abspath +from pathlib import Path +from subprocess import run + + +def get_hash(path): + """Return the hash of the given file.""" + hashobj = hashlib.sha512() + + with open(path, "rb") as file: + hashobj.update(file.read()) + + return hashobj.hexdigest() + + +def get_hashes(paths): + """Return a dict mapping the given files to their hashes.""" + return {abspath(path): get_hash(abspath(path)) for path in paths} + + +def sync(src_files): + cached_hashes_path = Path(environ["VIRTUAL_ENV"]) / "pip_sync_faster.json" + + try: + with open(cached_hashes_path, "r", encoding="utf-8") as handle: + cached_hashes = json.load(handle) + except FileNotFoundError: + cached_hashes = {} + + hashes = get_hashes(src_files) + + if hashes == cached_hashes: + return + + # The hashes did not match the cached ones. This can happen if: + # + # * This is the first time that pip-sync-faster has been called for this venv + # * One or more of the requirements files has changed + # * pip-sync-faster was called with a different set of requirements files + + run(["pip-sync", *sys.argv[1:]], check=True) + + # Replace the cached hashes file with one containing the correct hashes for + # the requirements files that pip-sync-faster was called with this time. + with open(cached_hashes_path, "w", encoding="utf-8") as handle: + json.dump(hashes, handle) diff --git a/tests/functional/pip_sync_faster_test.py b/tests/functional/pip_sync_faster_test.py deleted file mode 100644 index b76ddae..0000000 --- a/tests/functional/pip_sync_faster_test.py +++ /dev/null @@ -1,2 +0,0 @@ -def test_it(): - assert True diff --git a/tests/unit/pip_sync_faster/cli_test.py b/tests/unit/pip_sync_faster/cli_test.py new file mode 100644 index 0000000..3da2889 --- /dev/null +++ b/tests/unit/pip_sync_faster/cli_test.py @@ -0,0 +1,37 @@ +from importlib.metadata import version +from subprocess import CalledProcessError + +import pytest + +from pip_sync_faster.cli import cli + + +def test_cli(sync): + exit_code = cli(["requirements/dev.txt", "--foo", "bar"]) + + sync.assert_called_once_with(["requirements/dev.txt"]) + assert not exit_code + + +def test_version(capsys): + exit_code = cli(["--version"]) + + assert ( + capsys.readouterr().out.strip() + == f"pip-sync-faster, version {version('pip-sync-faster')}" + ) + assert not exit_code + + +def test_if_pip_sync_fails(sync): + sync.side_effect = CalledProcessError(23, ["pip-sync"]) + + exit_code = cli(["requirements/dev.txt"]) + + # It echoes pip-sync's exit code. + assert exit_code == 23 + + +@pytest.fixture(autouse=True) +def sync(mocker): + return mocker.patch("pip_sync_faster.cli.sync", autospec=True) diff --git a/tests/unit/pip_sync_faster/main_test.py b/tests/unit/pip_sync_faster/main_test.py deleted file mode 100644 index 966d9c4..0000000 --- a/tests/unit/pip_sync_faster/main_test.py +++ /dev/null @@ -1,3 +0,0 @@ -class TestHelloWorld: - def test_it(self): - assert True diff --git a/tests/unit/pip_sync_faster/sync_test.py b/tests/unit/pip_sync_faster/sync_test.py new file mode 100644 index 0000000..246a025 --- /dev/null +++ b/tests/unit/pip_sync_faster/sync_test.py @@ -0,0 +1,139 @@ +import json +import sys + +import pytest + +from pip_sync_faster import sync + + +class TestPipSyncFaster: + def test_if_the_hashes_match_it_doesnt_call_pip_sync( + self, cache_hashes, get_hashes, run, requirements_files + ): + # Make sure that all the cached hashes are present and matching. + cache_hashes(get_hashes(requirements_files)) + + sync.sync(requirements_files) + + run.assert_not_called() + + def test_if_theres_no_cached_hashes( + self, assert_hashes_cached, requirements_files, run + ): + sync.sync(requirements_files) + + run.assert_called_once_with(["pip-sync", *sys.argv[1:]], check=True) + assert_hashes_cached(requirements_files) + + def test_if_theres_one_wrong_hash( + self, assert_hashes_cached, cache_hashes, get_hashes, requirements_files, run + ): + # Make the cached hashes file contain one non-matching hash. + hashes = get_hashes(requirements_files) + hashes[list(hashes.keys())[0]] = "non_matching_hash" + cache_hashes(hashes) + + sync.sync(requirements_files) + + run.assert_called_once_with(["pip-sync", *sys.argv[1:]], check=True) + assert_hashes_cached(requirements_files) + + def test_if_theres_one_missing_hash( + self, assert_hashes_cached, cache_hashes, get_hashes, requirements_files, run + ): + # Make the cached hashes file be missing one hash. + cache_hashes(get_hashes(requirements_files[1:])) + + sync.sync(requirements_files) + + run.assert_called_once_with(["pip-sync", *sys.argv[1:]], check=True) + assert_hashes_cached(requirements_files) + + def test_if_theres_a_different_files_hash_cached( + self, assert_hashes_cached, cache_hashes, get_hashes, requirements_files, run + ): + # The cache contains correct hashes for both dev.txt and format.txt. + cache_hashes(get_hashes(requirements_files)) + + # pip-sync-faster is called with just format.txt. + sync.sync(requirements_files[1:]) + + # It should call pip-sync: the last time that pip-sync-faster was + # called (and updated the cache) it was called with both dev.txt and + # format.txt. Now it's being called with just format.txt. Any + # requirements that're in dev.txt but not in format.txt need to be + # removed from the venv. + run.assert_called_once_with(["pip-sync", *sys.argv[1:]], check=True) + # It should update the cache to contain only format.txt. + assert_hashes_cached(requirements_files[1:]) + + @pytest.fixture + def cached_hashes_path(self, tmp_path): + """Return the path where pip-sync-faster will look for its cached hashes file.""" + return tmp_path / "pip_sync_faster.json" + + @pytest.fixture + def assert_hashes_cached(self, cached_hashes_path, get_hashes): + def assert_hashes_cached(requirements_files): + """Assert the cache contains correct hashes for the given requirements files.""" + with open(cached_hashes_path, "r", encoding="utf-8") as handle: + cached_hashes = json.load(handle) + assert cached_hashes == get_hashes(requirements_files) + + return assert_hashes_cached + + @pytest.fixture + def cache_hashes(self, cached_hashes_path): + def cache_hashes(hashes): + """Cache the given hashes for the given paths.""" + with open(cached_hashes_path, "w", encoding="utf-8") as handle: + json.dump(hashes, handle) + + return cache_hashes + + @pytest.fixture + def requirements_files(self, tmp_path): + """Create the test requirements files and return their paths.""" + dev_txt_path = tmp_path / "dev.txt" + with open(dev_txt_path, "w", encoding="utf8") as handle: + handle.write("This is a fake dev.txt requirements file.") + + format_txt_path = tmp_path / "format.txt" + with open(format_txt_path, "w", encoding="utf8") as handle: + handle.write("This is a fake format.txt requirements file.") + + return [str(dev_txt_path), str(format_txt_path)] + + @pytest.fixture + def get_hashes(self, requirements_files): + dev_txt_path = requirements_files[0] + format_txt_path = requirements_files[1] + + hashes = { + dev_txt_path: "a184dca1232bd16942dbefb72782abce7b251055e59be5458f22a6e25ad3a7bec4579a6f6c1c41953c4440dbe66fe97a7dcb47c36350e94412fc9814a650556a", + format_txt_path: "e4837471e0008e297f2d4e37074f7b59b96cea821127ab825876bd76b7b079a0a80ff1e7ed50801295aa109b3300411b30fef42bec5346087aad63e9778ed806", + } + + def get_hashes(paths): + """Return a dict mapping the given paths to their correct hashes.""" + return {path: hashes[path] for path in paths} + + return get_hashes + + +@pytest.fixture(autouse=True) +def environ(mocker, tmp_path): + return mocker.patch.dict( + sync.environ, + { + # There isn't actually a virtualenv here but it doesn't matter: + # this is just where pip-sync-faster will look for and create + # its cached hashes file. + "VIRTUAL_ENV": str(tmp_path.absolute()), + }, + ) + + +@pytest.fixture(autouse=True) +def run(mocker): + return mocker.patch("pip_sync_faster.sync.run", autospec=True) diff --git a/tox.ini b/tox.ini index 2efc097..77e7b81 100644 --- a/tox.ini +++ b/tox.ini @@ -30,6 +30,7 @@ deps = lint,tests,functests: factory-boy lint,tests,functests: h-matchers lint,template: cookiecutter + lint,tests: pytest-mock depends = coverage: tests,py{39,38}-tests commands =