Allow non lowercase extras #4901

GabrielC101 · 2017-12-02T04:28:29Z

GabrielC101 · 2017-12-02T16:18:15Z

This is a bug, but the documentation is very unclear about whether names are case sensitive.

Behavior is properly documented in setuptools: https://github.com/pypa/setuptools/blob/master/docs/pkg_resources.txt#L663

Except when it's not:
https://github.com/pypa/setuptools/blob/master/docs/setuptools.txt#L689

High priority solution: update documentation for setuptools and pip to make it clear only lowercase names should be used.

GabrielC101 · 2017-12-02T16:32:21Z

Additional problem: should all names simply be converted to lowercase in pip?

Are there security issues here?

If someone created a myPackage module, and a bad actor created mypackage, the person installing myPackage would be extremely vulnerable.

Currently it would just fail. I'm not sure why there's a simple "does not exist" failure, rather than installing the lowercase version. But that seems to be a good thing, in regards to security.

Edit: for some issues an extra name will be in it's initial form, but for most it will be in it's lower case/ normalized form. Find out when pip uses the initial form. That's the functional (non-documentation) bug.

GabrielC101 · 2017-12-03T15:52:33Z

Research note: When setuptools/pip generates a wheel, requires.txt lists the extras using the initial, unsanitized, name.

Continuing research.

GabrielC101 · 2017-12-03T18:45:29Z

Trying to install a package with setup arg of:
name='mytestpackage'
extras_require={'EXTRAS': [extra_dep_one, extra_dep_two]},

Able to replicate error only when installing via local devpi. The following work fine:
pip install .[EXTRAS]
pip install -e .[EXTRAS]

However,
pip install -i https://my-devpi-server.com:4040/root/dev/+simple/ mytestpackage[EXTRAS]
gives me
Ignoring <dependency_package>: markers 'extra == "EXTRAS"' don't match your environment

pradyunsg · 2017-12-03T19:41:52Z

Hi @GabrielC101!

Just wanted to say that this is extremely useful research you're doing here. I've known for a while that extras are not consistently handled throughout pip/setuptools.

IMO, extras should be normalised at the entry point where they come in from. That should then be passed internally only as normalised extras. That means, Extra-1 and extra_1 are the same.

I don't think there's a security issue here since package names follow the same normalisation rule.

GabrielC101 · 2017-12-03T20:54:36Z

Thank you @pradyunsg . Hopefully I'll figure this thing out.

benoit-pierre · 2017-12-04T02:43:14Z

Is the goal to do case sensitive lookups for extras (potentially allowing both rest and reST as different extras for the same package), or to make sure the lookup is always case insensitive, and using both rest and REST would work if the package as the reST extra?

pradyunsg · 2017-12-04T03:52:13Z

Case insensitive, I hope.

…

On Mon, 4 Dec 2017, 08:13 Benoit Pierre, ***@***.***> wrote: Is the goal to do case insensitive lookups for extras (potentially allowing both rest and reST as different extras for the same package), or to make sure the lookup is always case insensitive, and using both rest and REST would work if the package as the reST extra? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4901 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADH7SaWIZtEByAgq-tIO4wLOmqU-LVrgks5s81xEgaJpZM4QzKvO> .

benoit-pierre · 2017-12-04T03:54:46Z

I hope too.

GabrielC101 · 2017-12-04T05:56:04Z

Case insensitive would be easiest to implement. A normalized extra is lower case. Changing that would be hard/dangerous.

GabrielC101 · 2017-12-05T04:03:42Z

Note: this problem is already mentioned in a TODO in a vendored file.

pip/pip/_vendor/packaging/requirements.py

Line 88 in 022248f

# TODO: Can we normalize the name and extra name?

GabrielC101 · 2017-12-05T04:33:33Z

It seems this is an issue with a vendored file. I've introduced a pull request in packaging to address it.

I'm not sure how you update vendored files in this repo . . . should I submit a similar fix?

pradyunsg · 2017-12-05T04:40:40Z

When packaging releases the fix, we'll pull in the newer release... It'll be done by manually running an invoke task later. We do that every few weeks. You don't need to do anything on this repository for a change made in packaging.

…

On Tue, 5 Dec 2017, 10:03 Gabriel Curio, ***@***.***> wrote: It seems this is an issue with a vendored file. I've introduced a pull request in packaging to address it. I'm not sure how you update vendored files in this repo . . . should I submit a similar fix? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4901 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADH7SSIc1Bko2uzt-S0OyDGYVID6tScBks5s9MeggaJpZM4QzKvO> .

GabrielC101 · 2017-12-05T14:05:54Z

Research Note: The original error, Ignoring <dependency_package>: markers 'extra == "EXTRAS"' don't match your environment does not appear when all of the dependencies associated with the extra are already installed.

If the setup.py file contains extras_required={'EXTRA': ['requirementname']} and pip freeze prints out requirementname, there is no error when I run pip install packagename[EXTRA].

However, if pip freeze does not print out requirementname, the error, Ignoring <dependency_package>: markers 'extra == "EXTRAS"' don't match your environment, reappears when I attempt pip install packagename[EXTRA].

GabrielC101 · 2017-12-06T00:07:43Z

Research Note: The original error, Ignoring <dependency_package>: markers 'extra == "EXTRAS"' don't match your environment does not appear when installing with the flag --no-cache-dir.

Nor does it appear when installing (without --no-cache-dir) the first time after deleting the contents of my cache. It does appear every subsequent time.

Seems safe to say this error only appears when installing from cache.

GabrielC101 · 2017-12-06T02:49:32Z

Research note:

I usually build and deploy packages to devpi using the following commands:
python setup.py sdist
devpi upload

When I switch to:
python setup.py bdist_wheel --universal
devpi upload --formats bdist_wheel

I get the error regardless of whether I'm install from cache or devpi. Previously, I only got it when installing from cache.

Edit: I'm shocked by this. I thought it would be the opposite - non-wheels generate the error. I'm 90% this is caused during wheel building due to the fact that in packaging.requirements.Requirement doesn't force extra names into lower case.

GabrielC101 · 2017-12-06T04:48:09Z

The issue seems to be that packaging/pyparser consider parse the value using the Marker object in packaging.markers.

GabrielC101 · 2017-12-07T01:46:10Z

Research note: original error only shows up for extras that pip should be attempting to install.
For example, if I enter pip install package_name[EXTRA_ONE], yet setup.py contains both EXTRA_ONE and EXTRA_TWO as dicts given to extra_requires, the error warning will only show up for EXTRA_ONE.

Presumably pip checks for the presence of both extras. Yet only the right one throws an error warning.

GabrielC101 · 2017-12-10T01:07:14Z

Research note:
Tried reproducing error with different versions.
pip==8.1.1 - no error
pip==8.1.2 - no error
pip==9.0.0 - error occurs, but output different color

GabrielC101 · 2017-12-10T01:10:48Z

Seems the issue was introduced by pull request #4051 in commit 8f171cd.

GabrielC101 · 2017-12-10T02:36:50Z

The problem arises when an extra is evaluated as a marker. Extras can be markers. They can also be extras. Conceptually, it's the difference between Requirement.extras and Requirment.markers['extra'].

Marker object contains an evaluate method that evaluates whether it is true. The only reason an extra will be evaluated as True is if a {"extra": "extraname"} is passed to the evaluate function.

When the marker is evaluated, it exists in it's whatever case form. It seems that there are no special parsing rules for markers with the extra value. So extra == Name is extra == Name. However pip assumes the marker name is properly parsed, and gives the environment in lower case when evaluate is called.

In order to fix this, package will have to be altered to parse markers with the "extra" value in lower case form.

GabrielC101 · 2017-12-12T20:53:18Z

Note to self: When a Requirement has an extra (Requirement.extra) the extra is optional. When a Requirement has a marker, a true evaluation is mandatory.

This problem isn't directly arising from the installation from the primary package, but from the dependent that's being installed because the primary package has an extra.

So if a package is named package-name, and setup.py has extra_requires={extra-name: [extra-package-name]}, then package-name.extra == extra_name, but extra-package-name.markers['extra'] == extra-name.

GabrielC101 · 2017-12-12T22:15:29Z

Problem: When resolver attempts to install a subrequirement, the parent Requirement.extras (+ "") become the Requirement.marker['extras'] for the subreq.

Pip parses extras correctly, but markers not at all.

This problem actually begins when a wheel is parsed by the DistInfoDistibution class. It parses Provides-Extra values as strings, but Requires-Dist are parsed as requirements, with the extra name as a marker, into the Requirement subclass class in pkg_resources.

Provides-Extras values are immediately normalized, however the old unnormalized version is retained and used to construct the dependency mapping - including evaluating markers. However, as soon as an dependencies are calculated, the extra name is normalized and stored as the key by which to access the partially semi-normalized pkg_resources.Requirement.

The dependency mapping at this point is "wrong". The extras are normalized, but the corresponding requirements (and their markers) are not.

When a pip user specifies an extra via command line, the extra name is properly parsed, and it is properly matched to the correct dependencies (whose names are also correctly parsed).

However, when the dependency is being installed, it checks to make sure all the markers are evaluated to True. Because the expected dependency is derived from the parent's extra, the expected extra is properly normalized. Because the markers are the fruit of pkg_resources.Requirement subclass, they are not parsed. Therefore, unless the markers.extras are already in parsed format, they don't match and fail to install.

When the initial error says markers 'extra == "reST"' don't match your environment, "extra" is not the extra from from pip install package[extra]. It's actually EXTRA from the METADATA file's Provides-Extra entry - after being normalized via safe_extra.

Provides-Extra is properly parsed so reST becomes rest. However Requires-Dist is not properly parsed, so reST stays as reST. The extra from the initial error is the the extra from Requires-Dist.

Ok . . . so how to fix this?

Option 1: Better parsing of Requires-Dist entries. These are parsed as Requirements - not packaging requirements, but a subclass in pkg_resources. Apparently pkg_resources doesn't like that packaging.Requirment doesn't normalize names or extras, so it has it's own subclass that does the parsing. Unfortunately, it parses names and extras, but not markers. This could be fixed in pkg_resources.
Option 2: Fix marker parsing in packaging. Same as option 1, but somewhere else. Seeing as the name-parsing and extra-parsing functionality was added in pkg_resources, it makes sense to parse the markers there too. But if there's interest in updating packages, marker parsing could be a part of that.
Option 3: Stop normalizing extra names in pip. Remove safe_extra. Cases are now sensitive. Bad.
Option 4: Stop evaluating markers. Bad.

Edit: What if we disable markers, but only when the markers are evaluating extras? Is there really a need for a requirement to evaluate the extra marker? The requirement is being installed because it was found in the dependency mapper via the parsed extra name key. We know that the extra should be true.

GabrielC101 · 2017-12-13T02:06:55Z

I've created a version that prevents subrequisites from checking their parent's extras via markers. Markers are still checked, but no extra_requirements are provided, and the resulting error is handled out. All other markers still function.

On the one hand, an extra isn't an environment variable. One requisite might have an extra, while another doesn't. The same isn't true for OS or Python version.

On the other hand, the way markers and 'Dist-Required' are parsed really needs to improve.

It works.

Let me get some tests in place before it's reviewed.

GabrielC101 · 2017-12-14T18:37:02Z

Is there any use case where an InstallRequirement.markers[0]["extra"] == extra_name should evaluate as False? Where pip has decided to install a package, but the install should be aborted because the correct extra hasn't been provided?

pradyunsg · 2017-12-26T23:38:48Z

news/4617.bugfix

@@ -0,0 +1 @@
+Stop unnecessary extra evaluations causing from causing errors.


This needs to be reworded.

pradyunsg · 2017-12-26T23:39:27Z

src/pip/_internal/req/req_install.py

+            try:
+                return self.markers.evaluate()
+            except UndefinedEnvironmentName:
+                return True


Could you add a comment here about why the flow is the way it is?

… extras to evaluate as True.

pradyunsg

This doesn't seem right but I'm not comfortable putting weight behind that assertion without spending more time to identify what feels wrong; which I don't have right now.

Sorry. :\

pradyunsg · 2018-01-19T19:57:55Z

tests/functional/test_install_extras.py

@@ -126,3 +127,22 @@ def test_install_special_extra(script):
    assert (
        "Could not find a version that satisfies the requirement missing_pkg"
    ) in result.stderr, str(result)
+
+
+@pytest.mark.skipif(sys.version_info < (3, 0),


Why is this test dependant on Py3?

django-rest-swagger[reST]==0.3.10 requires python3. There's probably a better way to test this problem, but this works.

I don't really like having a test that:

Depends on an external package that "happens" to have this issue (I describe it like that because we don't appear to have established precisely what causes the issue).

Can only be tested on Python 3, even though the problem can happen on Python 2.

May stop working if the external package changes.

I'd rather we tested this using an artificially-constructed example that we ship with the test suite, so we can control it better.

pfmoore · 2018-01-19T20:31:55Z

There's a lot of analysis here that I haven't really read through, but my instinct is that as pip (and PyPA projects in general) are trying to move away from implementation-defined behaviour and towards standards-based, the correct way forward for this change is to propose a change to the relevant standards to formally state whether extras are case-sensitive or not. Once there's a standard, agreeing any necessary changes to the tools to conform to that standard is easy. As things stand, however, it's hard to decide whether the change is "right".

GabrielC101 · 2018-01-19T21:31:49Z

@pfmoore

The "extras are case insensitive" rule seems established. pip strips extras of their case all the time. Any ambiguity is in the wheel METADATA v 1.3 specification for Provides-Extra.

The reason this error is manifesting is because 99% extra names are stripped of case, but every now and then the 1% happens.

In fact, that's why it's impossible to install an extra with mixed case names (from a wheel). Before attempting to install the extra, it will be stripped of all case. Due to a weird error the dependency mapping dictionary keeps the value - but not the key - in the original case.

The purpose of this PR isn't add a feature, but to destroy a bug (#4617). Resolving this bug will require insensitive or sensitive extra names. The current behavior of "extra refuses to install no matter what name is typed in" should be changed.

pfmoore · 2018-01-20T11:32:50Z

src/pip/_internal/req/req_install.py

+            try:
+                return self.markers.evaluate()
+            except UndefinedEnvironmentName:
+                return True


This doesn't really make sense to me. It seems to just be hiding an error rather than fixing it. The comment seems to be trying to justify doing so, but "in certain situations" doesn't exactly sound like we know what's going on here :-(

pfmoore · 2018-01-20T11:39:37Z

OK, thanks for the clarification, although I couldn't find anything in the PEPs that said extras were case insensitive (you originally mentioned PEP 508, but I see you edited your comment).

My point here is that if extra names are case insensitive, they should be normalised (just like package names) so that they can be compared by simple equality. My preference would be for this to be done in the metadata itself, but I'd also be OK with pip normalising all extra names when it encounters them.

We've apparently already introduced bugs by incompletely fixing this issue (at least that's what I understand from the fact that #4617 was introduced by the fix for #3810) so I'd rather that we properly fix it once and for all this time.

pradyunsg · 2018-01-20T11:50:23Z

I'd also be OK with pip normalising all extra names when it encounters them.

+1

I'd go so far as to say that the right thing to do here is to always normalise the extras at the edge, when pip loads them from the metadata. That way when later potential future build backends spew out non-normalised data, pip still does the right thing.

The current logic to handle extras during resolution (in the master) ain't perfect but I'd rather have this PR do better normalisation instead of touching that logic (since the later seems unnecessary).

BrownTruck · 2018-03-02T01:45:01Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

benoit-pierre · 2018-09-14T15:08:20Z

I don't think this can/should be fixed in pip, see #4617 (comment).

pradyunsg · 2019-05-23T04:30:13Z

Closing this since it's bitrotten. Please file a new PR if this work gets updated.

GabrielC101 changed the title ~~Allow non lowercase extras~~ WIP: Allow non lowercase extras Dec 2, 2017

GabrielC101 mentioned this pull request Dec 5, 2017

Extra environment marker should evaluate correctly. pypa/packaging#116

Closed

GabrielC101 force-pushed the allow-non-lowercase-extras branch from 8b46531 to 573925d Compare December 10, 2017 02:50

GabrielC101 force-pushed the allow-non-lowercase-extras branch from a2fa14e to 4eb11e0 Compare December 13, 2017 00:55

GabrielC101 closed this Dec 15, 2017

pradyunsg added type: enhancement Improvements to functionality C: extras Handling optional dependencies labels Dec 26, 2017

pradyunsg reviewed Dec 26, 2017

View reviewed changes

GabrielC101 force-pushed the allow-non-lowercase-extras branch 6 times, most recently from 89358f6 to 5a40f76 Compare December 27, 2017 17:48

GabrielC101 added 5 commits December 27, 2017 11:58

Don't check extra markers.

d6c806a

extras_requested shouldn't be passed to sub requisites.

7aa42fd

Add bugfix report.

61bf856

Add unit test to check installation of non lower case extra.

e51b13f

Improve description of changes in news file.

31f947a

GabrielC101 force-pushed the allow-non-lowercase-extras branch from 5a40f76 to dce1e23 Compare December 27, 2017 18:00

Add documentation to explain how and why try/except clause is forcing…

7d3b57b

… extras to evaluate as True.

GabrielC101 force-pushed the allow-non-lowercase-extras branch from dce1e23 to 7d3b57b Compare December 27, 2017 18:04

pradyunsg reviewed Jan 19, 2018

View reviewed changes

pfmoore reviewed Jan 20, 2018

View reviewed changes

BrownTruck added the needs rebase or merge PR has conflicts with current master label Mar 2, 2018

pradyunsg closed this May 23, 2019

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 22, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 22, 2019

		@@ -0,0 +1 @@
		Stop unnecessary extra evaluations causing from causing errors.

Allow non lowercase extras #4901

Allow non lowercase extras #4901

Conversation

GabrielC101 commented Dec 2, 2017

GabrielC101 commented Dec 2, 2017 • edited Loading

GabrielC101 commented Dec 2, 2017 • edited Loading

GabrielC101 commented Dec 3, 2017

GabrielC101 commented Dec 3, 2017 • edited Loading

pradyunsg commented Dec 3, 2017

GabrielC101 commented Dec 3, 2017

benoit-pierre commented Dec 4, 2017 • edited Loading

pradyunsg commented Dec 4, 2017 via email

benoit-pierre commented Dec 4, 2017

GabrielC101 commented Dec 4, 2017

GabrielC101 commented Dec 5, 2017 • edited Loading

GabrielC101 commented Dec 5, 2017

pradyunsg commented Dec 5, 2017 via email

GabrielC101 commented Dec 5, 2017 • edited Loading

GabrielC101 commented Dec 6, 2017 • edited Loading

GabrielC101 commented Dec 6, 2017 • edited Loading

GabrielC101 commented Dec 6, 2017

GabrielC101 commented Dec 7, 2017

GabrielC101 commented Dec 10, 2017

GabrielC101 commented Dec 10, 2017

GabrielC101 commented Dec 10, 2017 • edited Loading

GabrielC101 commented Dec 12, 2017 • edited Loading

GabrielC101 commented Dec 12, 2017 • edited Loading

GabrielC101 commented Dec 13, 2017 • edited Loading

GabrielC101 commented Dec 14, 2017 • edited Loading

pradyunsg Dec 26, 2017

Choose a reason for hiding this comment

pradyunsg Dec 26, 2017

Choose a reason for hiding this comment

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg Jan 19, 2018

Choose a reason for hiding this comment

GabrielC101 Jan 20, 2018

Choose a reason for hiding this comment

pfmoore Jan 20, 2018

Choose a reason for hiding this comment

pfmoore commented Jan 19, 2018

GabrielC101 commented Jan 19, 2018 • edited Loading

pfmoore Jan 20, 2018

Choose a reason for hiding this comment

pfmoore commented Jan 20, 2018

pradyunsg commented Jan 20, 2018

BrownTruck commented Mar 2, 2018

benoit-pierre commented Sep 14, 2018

pradyunsg commented May 23, 2019

GabrielC101 commented Dec 2, 2017 •

edited

Loading

GabrielC101 commented Dec 2, 2017 •

edited

Loading

GabrielC101 commented Dec 3, 2017 •

edited

Loading

benoit-pierre commented Dec 4, 2017 •

edited

Loading

GabrielC101 commented Dec 5, 2017 •

edited

Loading

GabrielC101 commented Dec 5, 2017 •

edited

Loading

GabrielC101 commented Dec 6, 2017 •

edited

Loading

GabrielC101 commented Dec 6, 2017 •

edited

Loading

GabrielC101 commented Dec 10, 2017 •

edited

Loading

GabrielC101 commented Dec 12, 2017 •

edited

Loading

GabrielC101 commented Dec 12, 2017 •

edited

Loading

GabrielC101 commented Dec 13, 2017 •

edited

Loading

GabrielC101 commented Dec 14, 2017 •

edited

Loading

GabrielC101 commented Jan 19, 2018 •

edited

Loading