fix(pipstar): Handle dep appearing in extra both conditionally and unconditionally

[thirtyseven]

Fixes #3511

[gemini-code-assist]

Summary of Changes

Hello @thirtyseven, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the dependency resolution logic in pipstar to prevent redundant entries when a dependency is specified both as a direct requirement and as part of an 'extra'. By ensuring that dependencies already present in the main dependency list are not duplicated in the deps_select mechanism, this change improves the accuracy and robustness of dependency parsing for complex requires_dist specifications.

Highlights

• Dependency Resolution Fix: Addressed a bug in the deps function within pep508_deps.bzl where a dependency could be incorrectly duplicated in deps_select if it was present both as a direct dependency and within an 'extra' requirement.
• New Test Case: Introduced a new test, test_markers_with_extra, to validate the correct handling of dependencies that appear in requires_dist with and without 'extra' markers, ensuring no redundant entries are created.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a bug where a dependency appearing multiple times with different markers under an 'extra' was not handled correctly. The change ensures that if a dependency becomes unconditional, it is not also listed as a conditional dependency. The fix is effective and is well-supported by a new test case that validates the corrected behavior.

[rickeylev]

Thanks for the fix! And double thanks for adding a test, too!

@aignas I think this LGTM, so I'll approve and queue for merge. It looks like the logic to loop over the requirements line kept two lists (one for unconditional deps, one for conditional deps). In this case, the requirements ended up such that the same entry occurred twice, but one was considered unconditional and the other conditional. I'm not sure why that would occur; my understanding is that requirement lines for the same dep should be mutually exclusive, but I'm not too surprised to see a case where that isn't true. Also note that "unconditional" here means "unconditional with respect to an extras condition" (in the requirements, it has foo; extra=X and foo; platform=darwin and extra=X).

[thirtyseven]

@rickeylev you can inspect any of the Darwin wheels for Ray to see an example of this in the wild. It’s using the more specific requires-dist to enforce a negative version match on Darwin, which matters at dependency resolution time but not here. The test case is based on it.