Repository navigation
Stop extras from installing other people's packages #18
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.
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
Open
Coding-Dev-Tools
wants to merge
2
commits into
main
Choose a base branch
from
fix/install-points-at-strangers-packages
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,179 @@ | ||
| """Packaging regression tests for extras, version lookups, and install hints. | ||
|
|
||
| Guards three defects raised in review: | ||
| - Version checks queried the import name for the four tools whose distribution | ||
| names differ, so an installed tool reported "not installed". | ||
| - Tool extras must never resolve a bare PyPI name owned by someone else. | ||
| - Install hints used ``git+...git[extra]``, which pip rejects; in a PEP 508 | ||
| direct reference the extra must come before the ``@``. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
| import re | ||
| import sys | ||
| from devforge import TOOLS | ||
| from devforge.cli import app | ||
| from pathlib import Path | ||
| from typer.testing import CliRunner | ||
| from unittest import mock | ||
|
|
||
| if sys.version_info >= (3, 11): | ||
| import tomllib | ||
| else: | ||
| import tomli as tomllib | ||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parent.parent | ||
| PYPROJECT = REPO_ROOT / "pyproject.toml" | ||
|
|
||
| # pyproject.toml declares name = "devforge-tools"; pip rejects a direct | ||
| # reference whose name disagrees with the built metadata. | ||
| DIST_NAME = "devforge-tools" | ||
| CLI_REPO_URL = "https://github.com/Coding-Dev-Tools/devforge-cli.git" | ||
| CLI_REPO = f"git+{CLI_REPO_URL}" | ||
| OUR_GITHUB_REQUIREMENT = " @ git+https://github.com/Coding-Dev-Tools/" | ||
|
|
||
| # Tools whose import name differs from their installed distribution name. | ||
| DIST_NAME_OVERRIDES = { | ||
| "deadcode": "deadcode-cli", | ||
| "envault": "rh-envault", | ||
| "schema": "schemaforge-cli", | ||
| "sql": "json2sql-cli", | ||
| } | ||
|
|
||
| # Dev-only tooling that legitimately comes from public PyPI. | ||
| ALLOWED_DEV_PACKAGES = {"pytest", "pyright", "tomli"} | ||
|
|
||
| NAME_PATTERN = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*") | ||
|
|
||
| runner = CliRunner() | ||
|
|
||
|
|
||
| def _optional_dependencies() -> dict[str, list[str]]: | ||
| with PYPROJECT.open("rb") as handle: | ||
| return tomllib.load(handle)["project"]["optional-dependencies"] | ||
|
|
||
|
|
||
| def _requirement_name(requirement: str) -> str: | ||
| match = NAME_PATTERN.match(requirement.strip()) | ||
| assert match is not None, f"unparseable requirement: {requirement!r}" | ||
| return match.group(0).lower().replace("_", "-") | ||
|
|
||
|
|
||
| def _pep508_requirement(command: list[str]) -> str: | ||
| matches = [arg for arg in command if arg.startswith(f"{DIST_NAME}[")] | ||
| assert len(matches) == 1, f"expected one PEP 508 requirement in {command}, got {matches}" | ||
| return matches[0] | ||
|
|
||
|
|
||
| class TestExtrasInstallOurPackages: | ||
| """Every tool extra must pin a verified Coding-Dev-Tools git source.""" | ||
|
|
||
| def test_every_tool_extra_is_a_coding_dev_tools_git_reference(self): | ||
| extras = _optional_dependencies() | ||
| tool_extras = {name: reqs for name, reqs in extras.items() if name != "dev"} | ||
| assert tool_extras, "no tool extras found in pyproject.toml" | ||
| violations = [ | ||
| f"{extra}: {requirement}" | ||
| for extra, requirements in tool_extras.items() | ||
| for requirement in requirements | ||
| if OUR_GITHUB_REQUIREMENT not in requirement | ||
| ] | ||
| assert not violations, ( | ||
| "These extras do not point at a verified Coding-Dev-Tools git source, " | ||
| "so pip would resolve a bare PyPI name owned by someone else:\n" + "\n".join(violations) | ||
| ) | ||
|
|
||
| def test_tool_extras_match_the_tool_registry(self): | ||
| extras = _optional_dependencies() | ||
| assert {name for name in extras if name not in {"all", "dev"}} == set(TOOLS) | ||
|
|
||
| def test_all_extra_aggregates_every_tool_extra(self): | ||
| extras = _optional_dependencies() | ||
| expected = [ | ||
| requirement | ||
| for extra, requirements in extras.items() | ||
| if extra not in {"all", "dev"} | ||
| for requirement in requirements | ||
| ] | ||
| assert sorted(extras["all"]) == sorted(expected) | ||
|
|
||
| def test_dev_extra_only_uses_verified_pypi_packages(self): | ||
| extras = _optional_dependencies() | ||
| unexpected = [req for req in extras["dev"] if _requirement_name(req) not in ALLOWED_DEV_PACKAGES] | ||
| assert not unexpected, f"dev extra pulls unverified PyPI package(s): {unexpected}" | ||
|
|
||
|
|
||
| class TestDistributionNameMapping: | ||
| """Version lookups must query the distribution, not the import module.""" | ||
|
|
||
| @pytest.mark.parametrize(("tool", "dist"), sorted(DIST_NAME_OVERRIDES.items())) | ||
| def test_registry_maps_module_name_to_distribution(self, tool, dist): | ||
| info = TOOLS[tool] | ||
| assert info["package"] != dist | ||
| assert info.get("dist", info["package"]) == dist | ||
|
|
||
| @pytest.mark.parametrize(("tool", "dist"), sorted(DIST_NAME_OVERRIDES.items())) | ||
| def test_versions_queries_distribution_not_module(self, tool, dist): | ||
| with mock.patch("devforge.cli._pip_version", return_value="1.2.3") as version: | ||
| result = runner.invoke(app, ["versions", tool]) | ||
| assert result.exit_code == 0 | ||
| version.assert_called_once_with(dist) | ||
| assert "v1.2.3" in result.stdout | ||
|
|
||
| def test_versions_queries_package_name_without_override(self): | ||
| with mock.patch("devforge.cli._pip_version", return_value="1.2.3") as version: | ||
| result = runner.invoke(app, ["versions", "guard"]) | ||
| assert result.exit_code == 0 | ||
| version.assert_called_once_with(TOOLS["guard"]["package"]) | ||
|
|
||
| def test_every_tool_extra_installs_the_name_the_version_check_queries(self): | ||
| extras = _optional_dependencies() | ||
| mismatches = [] | ||
| for tool, info in TOOLS.items(): | ||
| expected = {info.get("dist", info["package"])} | ||
| installed = {_requirement_name(req) for req in extras[tool]} | ||
| if installed != expected: | ||
| mismatches.append(f"{tool}: installs {sorted(installed)}, version check queries {sorted(expected)}") | ||
| assert not mismatches | ||
|
|
||
|
|
||
| class TestInstallHintsUsePep508: | ||
| """Hints and pip invocations must use ``name[extra] @ git+...`` syntax.""" | ||
|
|
||
| def test_tools_listing_hint(self): | ||
| result = runner.invoke(app, ["tools"]) | ||
| assert result.exit_code == 0 | ||
| assert f'pip install "{DIST_NAME}[all] @ {CLI_REPO}"' in result.stdout | ||
|
|
||
| def test_tools_listing_hint_has_no_extras_after_url(self): | ||
| result = runner.invoke(app, ["tools"]) | ||
| assert f"{CLI_REPO}[" not in result.stdout | ||
|
|
||
| @mock.patch("devforge.cli._is_tool_installed", return_value=False) | ||
| def test_dispatch_hint(self, _installed): | ||
| result = runner.invoke(app, ["guard"]) | ||
| assert result.exit_code == 1 | ||
| assert f'pip install "{DIST_NAME}[guard] @ {CLI_REPO}"' in result.stdout | ||
| assert f"{CLI_REPO}[" not in result.stdout | ||
|
|
||
| @mock.patch("devforge.cli.subprocess.run") | ||
| def test_install_all_hands_pip_a_pep508_requirement(self, pip_run): | ||
| pip_run.return_value = mock.MagicMock(returncode=0, stdout="", stderr="") | ||
| result = runner.invoke(app, ["install", "all"]) | ||
| assert result.exit_code == 0 | ||
| assert _pep508_requirement(pip_run.call_args[0][0]) == f"{DIST_NAME}[all] @ {CLI_REPO}" | ||
|
|
||
| @pytest.mark.parametrize("tool", sorted(TOOLS)) | ||
| @mock.patch("devforge.cli.subprocess.run") | ||
| def test_install_single_tool_hands_pip_a_pep508_requirement(self, pip_run, tool): | ||
| pip_run.return_value = mock.MagicMock(returncode=0, stdout="", stderr="") | ||
| result = runner.invoke(app, ["install", tool]) | ||
| assert result.exit_code == 0 | ||
| assert _pep508_requirement(pip_run.call_args[0][0]) == f"{DIST_NAME}[{tool}] @ {CLI_REPO}" | ||
|
|
||
| def test_readme_documents_the_same_valid_form(self): | ||
| readme = (REPO_ROOT / "README.md").read_text(encoding="utf-8") | ||
| assert f'pip install "{DIST_NAME}[all] @ {CLI_REPO}"' in readme | ||
| assert f"{CLI_REPO}[" not in readme |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a GitHub Release triggers
.github/workflows/publish.yml, these optional dependencies are emitted asRequires-Distdirect URLs and PyPI rejects the upload, so the release workflow cannot publish this project while the Git URLs remain here. Setuptools documents this PyPI restriction.Useful? React with 👍 / 👎.