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

Conversation

mathbou
Copy link
Contributor

@mathbou mathbou commented Mar 17, 2024

Recently, I run in the same problem described in #662. To avoid this, I propose a finer check for duplicates based on both name and specifier.

As stated in the issue, when the --disable-pip flag is used, we could consider that a full requirement resolution has been made. Knowing that, as long as specifiers matches, having duplicates is not a problem. If they don't match, we raise an error like before.

On the side, I also add a small fix for stdout/stderr reading in pip_audit/_subprocess.py. I don't know if it's specific to windows, but the fact that a size was specified, I had the process hanging indefinitely.

@woodruffw
Copy link
Member

Thanks for the patch @mathbou! I'll review this today.

@woodruffw woodruffw self-requested a review March 18, 2024 14:44
@woodruffw woodruffw added the component:dep-sources Dependency sources label Mar 18, 2024
pip_audit/_subprocess.py Outdated Show resolved Hide resolved
@woodruffw woodruffw added the needs-response Needs response from the reporter. label Mar 20, 2024
@mathbou mathbou requested a review from woodruffw May 10, 2024 18:01
@mathbou
Copy link
Contributor Author

mathbou commented Aug 22, 2024

It's been a while here, is there anything that prevent us to go further with this PR ? @woodruffw

@woodruffw
Copy link
Member

It's been a while here, is there anything that prevent us to go further with this PR ? @woodruffw

Nope, I've just been delayed in reviews, sorry 😅. I'll do another pass on this today.

(Thank you very much for keeping this PR alive and conflict-free!)

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

Thanks @mathbou, this looks good to me!

Could you add a CHANGELOG entry describing the bugfix here? The other entries in the file should serve as a reference for our preferred entry format 🙂

@mathbou
Copy link
Contributor Author

mathbou commented Aug 26, 2024

@woodruffw I updated the changelog, feel free to change it if it's not clear enough

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:dep-sources Dependency sources needs-response Needs response from the reporter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants