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

Use unmangled extra in DistInfoDistribution._compute_dependencies #578

Closed

Conversation

blueyed
Copy link

@blueyed blueyed commented May 9, 2016

With an extra of "GFK" this would not be found as "gfk".

I came up with this when investigating an issue with pip-tools / pip (pypa/pip#3661).

With an `extra` of "GFK" this would not be found as "gfk".
@jaraco
Copy link
Member

jaraco commented May 9, 2016

Hi @blueyed, thanks for the suggestion. I'm not sure I fully understand the implications of this change. I hope you can help.

So your use case suggests that something doesn't work in some circumstances, but as I walk through the tickets, I don't see a use case that I can readily use to replicate the failure (preferably through setuptools directly, without pip). Could you put something like that together to demonstrate the undesirable behavior?

The use of safe_extra seems relevant here. By lower-casing it, it's normalizing it, which by some measures would mean that the extras are compared case-insensitive. Perhaps that was the intention of the original code. Your change makes it specifically case sensitive. Another way to think about the issue is that marker.evaluate should compare markers case-insensitive. How do we know the implementation you suggest is preferable in the general case?

Something that will help answer these questions would be to include a unit test demonstrating the intended behavior and a changelog entry that goes along with this change explaining to the user what to expect differently as a result of this change (you can author it here or include it in CHANGES.rst).

@blueyed
Copy link
Author

blueyed commented May 9, 2016

Thanks for your in depth response!

marker.evaluate should compare markers case-insensitive

Good point. That is not being done in pip's vendorized version (https://github.com/pypa/pip/blob/67f534d3e36bd91e5e8f03615886d200786fb4b4/pip/_vendor/pkg_resources/__init__.py#L2873-L2891), setuptools 19.4.

Have you read pypa/pip#3661 - I could imagine that it would be fixed by updating setuptools / pkg_resources in pip after all.
I've only tried to update the code in that region, but that failed - probably because other parts would have to be updated, too.

@jaraco
Copy link
Member

jaraco commented May 10, 2016

I have read pypa/pip#3661, but it wasn't clear to me from there what the issue was there, and it referenced another issue.

I could imagine that it would be fixed by updating setuptools / pkg_resources in pip after all.

Perhaps, if you believe the current version of setuptools is adequate... but not if you're thinking of diverging the vendored code - that would be undesirable.

@jaraco jaraco closed this May 19, 2016
@jaraco
Copy link
Member

jaraco commented May 19, 2016

I welcome a follow-up proposal to retain case-insensitivity in the comparison, perhaps by updating the vendored dependency to a new version.

@mindw
Copy link
Contributor

mindw commented Jul 23, 2016

Below is a simple test case which reproduces the issue with hyphens. @blueyed patch fails in this case. This affects the wheel distribution - ed25519ll; extra == "faster-signatures". Perhaps implemeneting extras comparison instead of using normalizaion yield better results?

    def test_marker_evaluation_with_extras_hyphen(self):
        """Extras are also evaluated as markers at resolution time."""
        ad = pkg_resources.Environment([])
        ws = WorkingSet([])
        # Metadata needs to be native strings due to cStringIO behaviour in
        # 2.6, so use str().
        Foo = Distribution.from_filename(
            "/foo_dir/Foo-1.2.dist-info",
            metadata=Metadata(("METADATA", str("Provides-Extra: baz-lightyear\n"
                               "Requires-Dist: quux; extra=='baz-lightyear'")))
        )
        ad.add(Foo)
        assert list(ws.resolve(parse_requirements("Foo"), ad)) == [Foo]
        quux = Distribution.from_filename("/foo_dir/quux-1.0.dist-info")
        ad.add(quux)
        res = list(ws.resolve(parse_requirements("Foo[baz-lightyear]"), ad))
        assert res == [Foo, quux]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants