From 6db6ad8c4978a2135e319fb32cc003d7e2788e54 Mon Sep 17 00:00:00 2001 From: David Evans Date: Tue, 15 Oct 2024 09:37:23 +0100 Subject: [PATCH 1/6] Include newer Python versions in test matrix --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0251a71..83e34d0 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -7,7 +7,7 @@ jobs: strategy: matrix: os: [ubuntu-20.04, windows-2019] - python-version: ["3.8", "3.9", "3.10"] + python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"] runs-on: ${{ matrix.os }} name: Run test suite steps: From b5d97c85404b4099bebfff3561f21afddd7fd363 Mon Sep 17 00:00:00 2001 From: David Evans Date: Tue, 15 Oct 2024 09:57:15 +0100 Subject: [PATCH 2/6] fix: Make `setuptools` dependency explicit This is needed because of the dependency on `pkg_resources` which is itself needed due to some fandango with the OpenTelemetry packages: https://github.com/opensafely-core/opensafely-cli/blob/d9634cb79b/opensafely/__init__.py#L8-L14 We've not noticed this implicit dependency because all the environments we've been installing into thus far have already had `setuptools` installed. --- setup.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/setup.py b/setup.py index bcfaa6d..88af976 100644 --- a/setup.py +++ b/setup.py @@ -17,6 +17,9 @@ author="OpenSAFELY", author_email="tech@opensafely.org", python_requires=">=3.8", + install_requires=[ + "setuptools", + ], entry_points={"console_scripts": ["opensafely=opensafely:main"]}, classifiers=["License :: OSI Approved :: GNU General Public License v3 (GPLv3)"], project_urls={ From 52012d0eb80aa78cd31c72ba90c37fa79fef872d Mon Sep 17 00:00:00 2001 From: David Evans Date: Tue, 15 Oct 2024 13:26:30 +0100 Subject: [PATCH 3/6] fix: Don't attempt self-upgrade if installed with `uv` Instead we prompt the user with the appropriate `uv` command. --- opensafely/__init__.py | 9 +++++++-- opensafely/upgrade.py | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/opensafely/__init__.py b/opensafely/__init__.py index 76abbbf..4825306 100644 --- a/opensafely/__init__.py +++ b/opensafely/__init__.py @@ -65,10 +65,15 @@ def warn_if_updates_needed(argv): try: latest = upgrade.check_version() if latest: + upgrade_command = ( + "uv tool upgrade opensafely" + if upgrade.is_installed_with_uv() + else "opensafely upgrade" + ) print( f"Warning: there is a newer version of opensafely available ({latest})" - " - please upgrade by running:\n" - " opensafely upgrade\n", + f" - please upgrade by running:\n" + f" {upgrade_command}\n", file=sys.stderr, ) # if we're out of date, don't warn the user about out of date images as well diff --git a/opensafely/upgrade.py b/opensafely/upgrade.py index 9bc6df2..1184b47 100644 --- a/opensafely/upgrade.py +++ b/opensafely/upgrade.py @@ -28,6 +28,17 @@ def main(version): print(f"opensafely is already at version {version}") return 0 + if is_installed_with_uv(): + print( + "The OpenSAFELY tool has been installed using `uv` so cannot be directly" + " upgraded.\n" + "\n" + "Instead, please run:\n" + "\n" + " uv tool upgrade opensafely\n" + ) + return 1 + # Windows shennanigans: pip triggers a permissions error when it tries to # update the currently executing binary. However if we replace the binary # with a copy of itself (i.e. copy to a temporary file and then move the @@ -84,3 +95,10 @@ def check_version(): return latest else: return False + + +def is_installed_with_uv(): + # This was the most robust way I could think of for detecting a `uv` installation. + # I'm reasonably confident in its specificity. It's possible that a `uv` change will + # cause this to give false negatives, but the tests should catch that. + return Path(sys.prefix).joinpath("uv-receipt.toml").exists() From 8001b3ec6ddc2777f46d527aa4eacbe5acdfb5f6 Mon Sep 17 00:00:00 2001 From: David Evans Date: Tue, 15 Oct 2024 11:45:11 +0100 Subject: [PATCH 4/6] Factor package building code out of test --- tests/test_packaging.py | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 7595d62..882fab8 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -12,28 +12,14 @@ project_fixture_path = Path(__file__).parent / "fixtures" / "projects" -@pytest.mark.parametrize( - "package_type,ext", [("sdist", "tar.gz"), ("bdist_wheel", "whl")] -) -def test_packaging(package_type, ext, tmp_path): - project_root = Path(__file__).parent.parent - # This is pretty yucky. Ideally we'd stick all the build artefacts in a - # temporary directory but I can't seem to persuade setuptools to do this - shutil.rmtree(project_root / "dist", ignore_errors=True) - shutil.rmtree(project_root / "build", ignore_errors=True) - # Build the package - subprocess_run( - [sys.executable, "setup.py", package_type], - check=True, - cwd=project_root, - ) +@pytest.mark.parametrize("package_type", ["sdist", "bdist_wheel"]) +def test_packaging(package_type, tmp_path): + package_path = build_package(package_type) # Install it in a temporary virtualenv subprocess_run([sys.executable, "-m", "venv", tmp_path], check=True) # sdist requires wheel to build subprocess_run([tmp_path / BIN_DIR / "pip", "install", "wheel"], check=True) - - package = list(project_root.glob(f"dist/*.{ext}"))[0] - subprocess_run([tmp_path / BIN_DIR / "pip", "install", package], check=True) + subprocess_run([tmp_path / BIN_DIR / "pip", "install", package_path], check=True) # Smoketest it by running `--help` and `--version`. This is actually a more # comprehensive test than you might think as it involves importing @@ -55,6 +41,23 @@ def test_packaging(package_type, ext, tmp_path): ) +def build_package(package_type): + extension = {"sdist": "tar.gz", "bdist_wheel": "whl"}[package_type] + project_root = Path(__file__).parent.parent + # This is pretty yucky. Ideally we'd stick all the build artefacts in a + # temporary directory but I can't seem to persuade setuptools to do this + shutil.rmtree(project_root / "dist", ignore_errors=True) + shutil.rmtree(project_root / "build", ignore_errors=True) + # Build the package + subprocess_run( + [sys.executable, "setup.py", package_type], + check=True, + cwd=project_root, + ) + package_path = list(project_root.glob(f"dist/*.{extension}"))[0] + return package_path + + def subprocess_run(cmd_args, **kwargs): """ Thin wrapper around `subprocess.run` which ensures that any arguments which From 40a08146402ab6300e9475ccab84d0ab2305a2e7 Mon Sep 17 00:00:00 2001 From: David Evans Date: Tue, 15 Oct 2024 15:01:28 +0100 Subject: [PATCH 5/6] Fix version used in packaging tests The default contents of the `opensafely/VERSION` file used to be a string which was (as well as being an invalid version specifier) consiered lower than all our published versions. There's a comment to this effect in one of the tests, which expects that the `upgrade` command will therefore have something to do. However, in commit 83dda1359c5c this changed and the default contents became `99.99.99` i.e. _higher_ than every published version. This meant that the test no longer tested the upgrade process. That's fortunate because had it worked the Docker test that comes after it would have been testing the latest published version, not the version under test. We now do some horrible monkey business to temporarily change the version file during the packaging tests. I did try just setting the default to a low version, but that triggers a "there's a new version" prompt which breaks a bunch of the tests. --- tests/test_packaging.py | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 882fab8..2680d2d 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -6,14 +6,31 @@ import pytest +import opensafely + BIN_DIR = "bin" if os.name != "nt" else "Scripts" project_fixture_path = Path(__file__).parent / "fixtures" / "projects" +@pytest.fixture +def older_version_file(): + # This is really not very nice, but short of reworking the way versioning is handled + # (which I don't want to do at the moment) I can't think of another way. In order to + # build a package with the right version (both in the metadata and in the code + # itself) we need to temporarily update the VERSION file. + version_file_path = Path(opensafely.__file__).parent / "VERSION" + orig_contents = version_file_path.read_bytes() + try: + version_file_path.write_text("0.1") + yield + finally: + version_file_path.write_bytes(orig_contents) + + @pytest.mark.parametrize("package_type", ["sdist", "bdist_wheel"]) -def test_packaging(package_type, tmp_path): +def test_packaging(package_type, tmp_path, older_version_file): package_path = build_package(package_type) # Install it in a temporary virtualenv subprocess_run([sys.executable, "-m", "venv", tmp_path], check=True) @@ -27,9 +44,6 @@ def test_packaging(package_type, tmp_path): # vendoring and packaging, issues tend to show up at import time. subprocess_run([tmp_path / BIN_DIR / "opensafely", "run", "--help"], check=True) subprocess_run([tmp_path / BIN_DIR / "opensafely", "--version"], check=True) - # This always triggers an upgrade because the development version is always - # considered lower than any other version - subprocess_run([tmp_path / BIN_DIR / "opensafely", "upgrade", "1.7.0"], check=True) # only on linux, as that has docker installed in GH if sys.platform == "linux": @@ -40,6 +54,17 @@ def test_packaging(package_type, tmp_path): cwd=str(project_fixture_path), ) + # This always triggers an upgrade because the development version is always + # considered lower than any other version + result = subprocess_run( + [tmp_path / BIN_DIR / "opensafely", "upgrade"], + check=True, + capture_output=True, + text=True, + ) + assert "Attempting uninstall: opensafely" in result.stdout + assert "Successfully installed opensafely" in result.stdout + def build_package(package_type): extension = {"sdist": "tar.gz", "bdist_wheel": "whl"}[package_type] From 831cb66a28b9b78802b5a3cfc9c1cedaef6dae53 Mon Sep 17 00:00:00 2001 From: David Evans Date: Tue, 15 Oct 2024 13:28:09 +0100 Subject: [PATCH 6/6] Test installation using `uv` --- .github/workflows/tests.yml | 4 ++++ tests/test_packaging.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 83e34d0..480a3f3 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -17,6 +17,10 @@ jobs: uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} + - name: Install `uv` + uses: astral-sh/setup-uv@f3bcaebff5eace81a1c062af9f9011aae482ca9d # v3.1.7 + with: + version: "0.4.20" - name: Install dependencies run: | python -m pip install -r requirements.dev.txt diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 2680d2d..34f9eac 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -66,6 +66,35 @@ def test_packaging(package_type, tmp_path, older_version_file): assert "Successfully installed opensafely" in result.stdout +def test_installing_with_uv(tmp_path, older_version_file): + uv_bin = shutil.which("uv") + if uv_bin is None: + pytest.skip("Skipping as `uv` not installed") + + package_path = build_package("bdist_wheel") + bin_path = tmp_path / "bin" + uv_env = dict( + os.environ, + UV_TOOL_BIN_DIR=bin_path, + UV_TOOL_DIR=tmp_path / "tools", + ) + python_version = f"python{sys.version_info[0]}.{sys.version_info[1]}" + subprocess_run( + [uv_bin, "tool", "install", "--python", python_version, package_path], + env=uv_env, + check=True, + ) + # Basic smoketest + subprocess_run([bin_path / "opensafely", "run", "--help"], check=True) + subprocess_run([bin_path / "opensafely", "--version"], check=True) + # The `upgrade` command should prompt the user to use `uv upgrade` instead + result = subprocess_run( + [bin_path / "opensafely", "upgrade"], capture_output=True, text=True + ) + assert result.returncode != 0 + assert "uv tool upgrade opensafely" in result.stdout + + def build_package(package_type): extension = {"sdist": "tar.gz", "bdist_wheel": "whl"}[package_type] project_root = Path(__file__).parent.parent