Skip to content
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

_dependency_source, _cli, test: Do not override pip.conf unless explicitly specified via flags #565

Merged
merged 7 commits into from
Mar 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ All versions prior to 0.0.9 are untracked.

## [Unreleased]

### Changed

* Refactored `index-url` option to not override user pip config by default,
unless specified ([#565](https://github.com/pypa/pip-audit/pull/565))

## [2.5.3]

### Changed
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ optional arguments:
--index-url INDEX_URL
base URL of the Python Package Index; this should
point to a repository compliant with PEP 503 (the
simple repository API) (default:
https://pypi.org/simple/)
simple repository API); this will be resolved by pip
if not specified (default: None)
--extra-index-url URL
extra URLs of package indexes to use in addition to
`--index-url`; should follow the same rules as
Expand Down
19 changes: 10 additions & 9 deletions pip_audit/_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
from pip_audit import __version__
from pip_audit._audit import AuditOptions, Auditor
from pip_audit._dependency_source import (
PYPI_URL,
DependencySource,
DependencySourceError,
PipSource,
Expand Down Expand Up @@ -285,8 +284,7 @@ def _parser() -> argparse.ArgumentParser: # pragma: no cover
"--index-url",
type=str,
help="base URL of the Python Package Index; this should point to a repository compliant "
"with PEP 503 (the simple repository API)",
default=PYPI_URL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to keep this default in the CLI -- behaviorally it should remain the default, and having it appear as such in the --help render makes for a nicer UX 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So leaving this as the default means that it will always be explicitly passed to the pip install CLI, which results in the index-url specified in the pip.conf being ignored. I understand that it makes for a more friendly UX to specify as such, but would it be reasonable to convey that the url will be resolved by pip if not passed by the user explicitly?

If this is a deal breaker though, then I'll have to dig deeper into pip to determine how it is performing the index-url resolution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So leaving this as the default means that it will always be explicitly passed to the pip install CLI, which results in the index-url specified in the pip.conf being ignored. I understand that it makes for a more friendly UX to specify as such, but would it be reasonable to convey that the url will be resolved by pip if not passed by the user explicitly?

That sounds reasonable to me!

"with PEP 503 (the simple repository API); this will be resolved by pip if not specified",
)
parser.add_argument(
"--extra-index-url",
Expand Down Expand Up @@ -347,12 +345,14 @@ def _parse_args(parser: argparse.ArgumentParser) -> argparse.Namespace: # pragm


def _dep_source_from_project_path(
project_path: Path, index_urls: list[str], state: AuditState
project_path: Path, index_url: str, extra_index_urls: list[str], state: AuditState
) -> DependencySource: # pragma: no cover
# Check for a `pyproject.toml`
pyproject_path = project_path / "pyproject.toml"
if pyproject_path.is_file():
return PyProjectSource(pyproject_path, index_urls=index_urls, state=state)
return PyProjectSource(
pyproject_path, index_url=index_url, extra_index_urls=extra_index_urls, state=state
)

# TODO: Checks for setup.py and other project files will go here.

Expand All @@ -374,7 +374,7 @@ def audit() -> None: # pragma: no cover
if args.requirements is None:
if args.require_hashes:
parser.error("The --require-hashes flag can only be used with --requirement (-r)")
elif args.index_url != PYPI_URL:
elif args.index_url:
parser.error("The --index-url flag can only be used with --requirement (-r)")
elif args.extra_index_urls:
parser.error("The --extra-index-url flag can only be used with --requirement (-r)")
Expand Down Expand Up @@ -408,15 +408,15 @@ def audit() -> None: # pragma: no cover
state = stack.enter_context(AuditState(members=actors))

source: DependencySource
index_urls = [args.index_url] + args.extra_index_urls
if args.requirements is not None:
req_files: list[Path] = [Path(req.name) for req in args.requirements]
source = RequirementSource(
req_files,
require_hashes=args.require_hashes,
no_deps=args.no_deps,
skip_editable=args.skip_editable,
index_urls=index_urls,
index_url=args.index_url,
extra_index_urls=args.extra_index_urls,
state=state,
)
elif args.project_path is not None:
Expand All @@ -426,7 +426,8 @@ def audit() -> None: # pragma: no cover
# Determine which kind of project file exists in the project path
source = _dep_source_from_project_path(
args.project_path,
index_urls,
args.index_url,
args.extra_index_urls,
state,
)
else:
Expand Down
17 changes: 9 additions & 8 deletions pip_audit/_dependency_source/pyproject.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,7 @@
from packaging.requirements import Requirement
from packaging.specifiers import SpecifierSet

from pip_audit._dependency_source import (
PYPI_URL,
DependencyFixError,
DependencySource,
DependencySourceError,
)
from pip_audit._dependency_source import DependencyFixError, DependencySource, DependencySourceError
from pip_audit._fix import ResolvedFixVersion
from pip_audit._service import Dependency, ResolvedDependency
from pip_audit._state import AuditState
Expand All @@ -33,14 +28,20 @@ class PyProjectSource(DependencySource):
"""

def __init__(
self, filename: Path, index_urls: list[str] = [PYPI_URL], state: AuditState = AuditState()
self,
filename: Path,
index_url: str | None = None,
extra_index_urls: list[str] = [],
state: AuditState = AuditState(),
) -> None:
"""
Create a new `PyProjectSource`.

`filename` provides a path to a `pyproject.toml` file

`index_urls` is a list of package indices.
`index_url` is the base URL of the package index.

`extra_index_urls` are the extra URLs of package indexes.

`state` is an `AuditState` to use for state callbacks.
"""
Expand Down
19 changes: 9 additions & 10 deletions pip_audit/_dependency_source/requirement.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,7 @@
from packaging.specifiers import SpecifierSet
from pip_requirements_parser import InstallRequirement, InvalidRequirementLine, RequirementsFile

from pip_audit._dependency_source import (
PYPI_URL,
DependencyFixError,
DependencySource,
DependencySourceError,
)
from pip_audit._dependency_source import DependencyFixError, DependencySource, DependencySourceError
from pip_audit._fix import ResolvedFixVersion
from pip_audit._service import Dependency
from pip_audit._service.interface import ResolvedDependency
Expand All @@ -45,7 +40,8 @@ def __init__(
require_hashes: bool = False,
no_deps: bool = False,
skip_editable: bool = False,
index_urls: list[str] = [PYPI_URL],
index_url: str | None = None,
extra_index_urls: list[str] = [],
state: AuditState = AuditState(),
) -> None:
"""
Expand All @@ -63,15 +59,18 @@ def __init__(
`skip_editable` controls whether requirements marked as "editable" are skipped.
By default, editable requirements are not skipped.

`index_urls` is a list of of package indices.
`index_url` is the base URL of the package index.

`extra_index_urls` are the extra URLs of package indexes.

`state` is an `AuditState` to use for state callbacks.
"""
self._filenames = filenames
self._require_hashes = require_hashes
self._no_deps = no_deps
self._skip_editable = skip_editable
self._index_urls = index_urls
self._index_url = index_url
self._extra_index_urls = extra_index_urls
self.state = state
self._dep_cache: dict[Path, set[Dependency]] = {}

Expand Down Expand Up @@ -127,7 +126,7 @@ def _collect_from_files(self, filenames: list[os.PathLike]) -> Iterator[Dependen
ve_args.extend(["-r", str(filename)])

# Try to install the supplied requirements files.
ve = VirtualEnv(ve_args, self._index_urls, self.state)
ve = VirtualEnv(ve_args, self._index_url, self._extra_index_urls, self.state)
try:
with TemporaryDirectory() as ve_dir:
ve.create(ve_dir)
Expand Down
17 changes: 11 additions & 6 deletions pip_audit/_virtual_env.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

from packaging.version import Version

from ._dependency_source import PYPI_URL
from ._state import AuditState
from ._subprocess import CalledProcessError, run

Expand Down Expand Up @@ -44,7 +43,8 @@ class VirtualEnv(venv.EnvBuilder):
def __init__(
self,
install_args: list[str],
index_urls: list[str] = [PYPI_URL],
index_url: str | None = None,
extra_index_urls: list[str] = [],
state: AuditState = AuditState(),
):
"""
Expand All @@ -57,13 +57,16 @@ def __init__(
ve = VirtualEnv(["-e", "/tmp/my_pkg"])
```

`index_urls` is a list of package indices to install from.
`index_url` is the base URL of the package index.

`extra_index_urls` are the extra URLs of package indexes.

`state` is an `AuditState` to use for state callbacks.
"""
super().__init__(with_pip=True)
self._install_args = install_args
self._index_urls = index_urls
self._index_url = index_url
self._extra_index_urls = extra_index_urls
self._packages: list[tuple[str, Version]] | None = None
self._state = state

Expand Down Expand Up @@ -157,8 +160,10 @@ def installed_packages(self) -> Iterator[tuple[str, Version]]:

@property
def _index_url_args(self) -> list[str]:
args = ["--index-url", self._index_urls[0]]
for index_url in self._index_urls[1:]:
args = []
if self._index_url:
args.extend(["--index-url", self._index_url])
for index_url in self._extra_index_urls:
args.extend(["--extra-index-url", index_url])
return args

Expand Down
10 changes: 8 additions & 2 deletions test/dependency_source/test_requirement.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,11 @@ def test_requirement_source_impossible_resolution(req_file):
def test_requirement_source_virtualenv_error(monkeypatch, req_file):
class MockVirtualEnv:
def __init__(
self, install_args: list[str], index_urls: list[str], state: AuditState
self,
install_args: list[str],
index_url: str,
extra_index_url: list[str],
state: AuditState,
) -> None:
pass

Expand Down Expand Up @@ -149,7 +153,9 @@ def test_requirement_source_url(req_file):
@pytest.mark.online
def test_requirement_source_multiple_indexes(req_file):
source = _init_requirement(
[(req_file(), "flask==2.0.1")], index_urls=[PYPI_URL, "https://test.pypi.org/simple/"]
[(req_file(), "flask==2.0.1")],
index_url=PYPI_URL,
extra_index_urls=["https://test.pypi.org/simple/"],
)
specs = list(source.collect())
assert ResolvedDependency("Flask", Version("2.0.1")) in specs
Expand Down