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

662 duplicates are not supported in requirements.txt when run with disable pip #749

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6adf10d
🐛 reqs with same name does not raise if matching specifier
Mar 16, 2024
a986445
🐛 ensure fixed file is closed before moving it
Mar 16, 2024
96306fa
🐛 fix pipe reading hanging indefinitely
Mar 16, 2024
8f852d1
✅ update tests
Mar 17, 2024
b968c85
✏️ fix typehint
Mar 17, 2024
0cf9887
⏪ re-set bufsize to 0
Mar 17, 2024
8ac2246
Revert "🐛 fix pipe reading hanging indefinitely"
Apr 1, 2024
c747317
Revert "🐛 ensure fixed file is closed before moving it"
Apr 1, 2024
016a7f0
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Apr 1, 2024
478c84d
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Apr 13, 2024
2219c12
Merge branch 'pypa:main' into 662-Duplicates-are-not-supported-in-req…
mathbou May 10, 2024
08d0890
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Jun 7, 2024
77da823
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Jul 22, 2024
0482630
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Aug 5, 2024
41c5f3c
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Aug 22, 2024
f0403fc
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Aug 23, 2024
43832a0
Update CHANGELOG.md
mathbou Aug 26, 2024
191bd59
Apply suggestions from code review
woodruffw Aug 26, 2024
fab3e95
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Aug 27, 2024
e8324bb
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Aug 30, 2024
ec412d8
Merge branch 'main' into 662-Duplicates-are-not-supported-in-requirem…
mathbou Sep 16, 2024
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
4 changes: 2 additions & 2 deletions pip_audit/_dependency_source/pyproject.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ def fix(self, fix_version: ResolvedFixVersion) -> None:
# Now dump the new edited TOML to the temporary file.
toml.dump(pyproject_data, tmp)

# And replace the original `pyproject.toml` file.
os.replace(tmp.name, self.filename)
# And replace the original `pyproject.toml` file.
os.replace(tmp.name, self.filename)
mathbou marked this conversation as resolved.
Show resolved Hide resolved


class PyProjectSourceError(DependencySourceError):
Expand Down
28 changes: 21 additions & 7 deletions pip_audit/_dependency_source/requirement.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,18 +225,23 @@ def _fix_file(self, filename: Path, fix_version: ResolvedFixVersion) -> None:
# Check ahead of time for anything invalid in the requirements file since we don't want to
# encounter this while writing out the file. Check for duplicate requirements and lines that
# failed to parse.
req_names: set[str] = set()
req_specifiers: dict[str, SpecifierSet] = dict()

for req in reqs:
if (
isinstance(req, InstallRequirement)
and (req.marker is None or req.marker.evaluate())
and req.req is not None
):
if req.name in req_names:
duplicate_req_specifier = req_specifiers.get(req.name)

if not duplicate_req_specifier:
req_specifiers[req.name] = req.specifier

elif duplicate_req_specifier != req.specifier:
woodruffw marked this conversation as resolved.
Show resolved Hide resolved
raise RequirementFixError(
f"package {req.name} has duplicate requirements: {str(req)}"
)
req_names.add(req.name)
elif isinstance(req, InvalidRequirementLine):
raise RequirementFixError(
f"requirement file {filename} has invalid requirement: {str(req)}"
Expand Down Expand Up @@ -294,7 +299,7 @@ def _collect_preresolved_deps(
"""
Collect pre-resolved (pinned) dependencies.
"""
req_names: set[str] = set()
req_specifiers: dict[str, SpecifierSet] = dict()
for req in reqs:
if not req.hash_options and require_hashes:
raise RequirementSourceError(f"requirement {req.dumps()} does not contain a hash")
Expand All @@ -315,12 +320,21 @@ def _collect_preresolved_deps(
if req.marker is not None and not req.marker.evaluate():
continue # pragma: no cover

# This means we have a duplicate requirement for the same package
if req.name in req_names:
duplicate_req_specifier = req_specifiers.get(req.name)

if not duplicate_req_specifier:
req_specifiers[req.name] = req.specifier

# We have a duplicate requirement for the same package
# but different specifiers, meaning a badly resolved requirements.txt
elif duplicate_req_specifier != req.specifier:
raise RequirementSourceError(
f"package {req.name} has duplicate requirements: {str(req)}"
)
req_names.add(req.name)
else:
# We have a duplicate requirement for the same package and the specifier matches
# As they would return the same result from the audit, there no need to yield it a second time.
continue
Copy link
Member

Choose a reason for hiding this comment

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

From CI: this line needs coverage!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is weird, coverage complains about this line only with py3.8 and 3.9, from 3.10 and above I get 100%.

Copy link
Member

Choose a reason for hiding this comment

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

This is probably a manifestation of nedbat/coveragepy#198, which was ultimately resolved in 3.10+ via PEP 626: https://peps.python.org/pep-0626/

The two solutions here are either to drop the else: ... continue or add a coverage ignore comment.


# NOTE: URL dependencies cannot be pinned, so skipping them
# makes sense (under the same principle of skipping dependencies
Expand Down
5 changes: 2 additions & 3 deletions pip_audit/_subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,8 @@ def run(args: Sequence[str], *, log_stdout: bool = False, state: AuditState = Au
# once `stdout` hits EOF, so we don't have to worry about that blocking.
while not terminated:
terminated = process.poll() is not None
# NOTE(ww): Buffer size chosen arbitrarily here and below.
stdout += process.stdout.read(4096) # type: ignore
stderr += process.stderr.read(4096) # type: ignore
stdout += process.stdout.read() # type: ignore
stderr += process.stderr.read() # type: ignore
woodruffw marked this conversation as resolved.
Show resolved Hide resolved
state.update_state(
f"Running {pretty_args}",
stdout.decode(errors="replace") if log_stdout else None,
Expand Down
24 changes: 24 additions & 0 deletions test/dependency_source/test_requirement.py
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,30 @@ def test_requirement_source_disable_pip_duplicate_dependencies(req_file):
[(req_file(), "flask==1.0\nflask==1.0")], disable_pip=True, no_deps=True
)

specs = list(source.collect())

# If the dependency list has duplicates, then converting to a set will reduce the length of the
# collection
assert len(specs) == len(set(specs))


def test_requirement_source_disable_pip_duplicate_dependencies_with_extras(req_file):
source = _init_requirement(
[(req_file(), "aiohttp==3.9\naiohttp[speedups]==3.9")], disable_pip=True, no_deps=True
)

specs = list(source.collect())

# If the dependency list has duplicates, then converting to a set will reduce the length of the
# collection
assert len(specs) == len(set(specs))


def test_requirement_source_disable_pip_duplicate_dependencies_diff_specifier(req_file):
source = _init_requirement(
[(req_file(), "flask==1.0\nflask==2.0")], disable_pip=True, no_deps=True
)

with pytest.raises(DependencySourceError):
list(source.collect())

Expand Down