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

Fix grouping of extras requirements in notpip #2142

Merged
merged 14 commits into from May 23, 2018

Conversation

Projects
2 participants
@altendky
Contributor

altendky commented May 4, 2018

No description provided.

altendky added some commits May 4, 2018

@altendky

This comment has been minimized.

Contributor

altendky commented May 4, 2018

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 4, 2018

What exactly is the goal

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 4, 2018

Are you saying the requirement names aren’t being identified as the same name

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 4, 2018

I suspect your fix belongs in pipenv.utils.get_requirement

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 4, 2018

Or else in piptools.resolver

@altendky

This comment has been minimized.

Contributor

altendky commented May 4, 2018

Stepping through the changed code it seemed to completely miscategorize the items. As noted in the ticket the twisted[windows_platform] dependency was properly ignored when it was the only twisted dependency, but when it was the second it would get processed despite being in Linux. If needed I can make sample input/output from before and after the patch right at that function.

@altendky

This comment has been minimized.

Contributor

altendky commented May 4, 2018

The filtering based on the sys_platform == "Windows" does occur somewhere else but I"m not sure where as I didn't have to look at it to fix the issue. This code placed the twisted[windows_platform] dependency not inside of the :sys_platform == "Windows" extras section but rather in the regular requires section before the patch so it was being installed even on Linux. But only with the other Twisted dependency line before it in the setup.py.

I looked around but didn't find a notpip project that seemed relevant. Did I just miss it?

@altendky

This comment has been minimized.

Contributor

altendky commented May 4, 2018

I think my refactor will cause another issue. If there's actually a notpip package somewhere I'll look at their tests etc. But regardless I'll update this later when I get a chance.

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 4, 2018

Yes. Notpip is a patched and renamed pip. The extras require syntax you mentioned is old and mostly unsupported.

twisted[windows_platform] is just twisted plus windows platform extras. I am fairly certain pip handles extras correctly which is why I am pointing you at the places where we have heavily modified resolver code, including extras groupings. But the real issue is caused by piptools. See #2123

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 4, 2018

There is a reason for the do not touch notices in the patched dir btw. There are going to be downstream consequences of a change to the modified resolver code. The issue in this case is just caused by piptools lowercasing a marker, but I couldn’t really follow what the issue was in the first place

@altendky

This comment has been minimized.

Contributor

altendky commented May 4, 2018

It's not clear that you understand the issue, but who knows, maybe the change will go elsewhere.

'twisted; platform_system != "Windows"',
'twisted[windows_platform]; platform_system == "Windows"',

With those deps the basic expectation is that on Linux the dependency twisted would be satisfied and on Windows the dependency twisted[windows_platform] would be satisfied. Pipenv successfully considers the platform specification when one or the other of those lines is present. So in Linux, with only the second line present in setup.py as an install_requires, no Twisted is installed at all. With the first line Twisted is sensibly installed for Linux. With both, the Windows-only dep gets installed. The [windows_platform] part is being handled fine. The : platform_system == "Windows" part is handled properly. Putting them together results in this changed block of code misparsing and the net result is it tries to install the Windows-only dependency on Linux. I'll go make up a demo now.

@altendky

This comment has been minimized.

Contributor

altendky commented May 4, 2018

Note how the old code results in a single 'extras' grouping :platform_system != \"Windows\" that actually includes both the not-Windows specified dependency and the Windows specified dependency and... the specification of [:platform_system == \"Windows\"] as if it were a dependency and not as a group? This seems obviously wrong and also doesn't work per my example in the ticket, unless there is some error in my testing (though it seemed completely consistent).

But, I'll take some time later and see if I can review the original source of this as well as explore the bug my modification might have.

https://repl.it/@altendky/CultivatedSardonicFactor-1

---- ['chardet', '[:platform_system != "Windows"]', 'twisted', '[:platform_system == "Windows"]', 'twisted[windows_platform]']
     old
{
    ":platform_system != \"Windows\"": [
        "twisted",
        "[:platform_system == \"Windows\"]",
        "twisted[windows_platform]"
    ]
}
     new
{
    ":platform_system != \"Windows\"": [
        "twisted"
    ],
    ":platform_system == \"Windows\"": [
        "twisted[windows_platform]"
    ]
}
@altendky

This comment has been minimized.

Contributor

altendky commented May 4, 2018

For reference, this function doesn't seem to be present upstream, assuming I'm looking in the right place.

https://github.com/pypa/pip/blob/master/src/pip/_internal/index.py

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 5, 2018

Right this is an issue internally in our modifications to the piptools resolver which first of all doesn’t even respect the uppercased version of platform system, and then either in piptools (which is how we get the dependency graph from a setup.py) or in our internal get_requirements function because this is where we take the string and break it apart to make into a lockfile entry. Pip works fine. I promise.

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 5, 2018

I understand the issue. This is one part of the codebase I know pretty well which is why I know this is probably not where we want to make these changes, since I think we can fix this in our own things.

@altendky

This comment has been minimized.

Contributor

altendky commented May 5, 2018

When only the == "Windows" dependency was present in the setup.py pipenv did properly ignore it and not install Twisted. This was shown in the ticket in the only-windows logs. Note the lack of any error and the pip freeze output showing only chardet and the testing sample 'project' having been installed.

https://gist.github.com/altendky/3adc9ed474beb9e1677bd4d402a5fcc7

  • Edit: I'm withdrawing the above comment because it doesn't prove the comparison works properly on Windows.

Do note that this function isn't in the original Pip file. I am not claiming that Pip has a bug, pipenv does. It comes from another patch already in the directory you mentioned. So, I presume that you are correct that Pip handles this scenario properly.

+ def get_extras_links(self, links):
+ requires = []
+ extras = {}
+
+ current_section = None
+
+ for link in links:
+ if not link:
+ current_section = None
+
+ if not current_section:
+ if not (link.startswith('[')):
+ requires.append(link)
+ else:
+ current_section = link[1:-1]
+ extras[current_section] = []
+ else:
+ extras[current_section].append(link)
+
+ return extras

The data coming into this function seems plausibly formatted so I'm skeptical the issue lies there and the data coming out of this function seems clearly wrong before this patch but looks useful afterwards. The change also resolves the initial symptom and didn't break any tests. Of course, that's not all it takes to make a change proper, but what am I missing? How is the original output of this function that I have shared in the examples superior to the changed output?

The potential bug I was concerned about earlier doesn't seem to be one. I was concerned that my implementation wouldn't handle multiple items that belong in the same group. As it turns out, that case is already handled elsewhere such that this code works. Consider the second example below.

https://repl.it/@altendky/CultivatedSardonicFactor-2

Another example 'test'
-------- ['chardet', '[:platform_system != "Windows"]', 'twisted', '[:platform_system == "Windows"]', 'twisted[windows_platform]']
     old
{
    ":platform_system != \"Windows\"": [
        "twisted",
        "[:platform_system == \"Windows\"]",
        "twisted[windows_platform]"
    ]
}
     new
{
    ":platform_system != \"Windows\"": [
        "twisted"
    ],
    ":platform_system == \"Windows\"": [
        "twisted[windows_platform]"
    ]
}


-------- ['chardet', 'requests', '[:platform_system != "Windows"]', 'twisted', 'pytest', '[:platform_system == "Windows"]', 'twisted[windows_platform]']
     old
{
    ":platform_system != \"Windows\"": [
        "twisted",
        "pytest",
        "[:platform_system == \"Windows\"]",
        "twisted[windows_platform]"
    ]
}
     new
{
    ":platform_system != \"Windows\"": [
        "twisted",
        "pytest"
    ],
    ":platform_system == \"Windows\"": [
        "twisted[windows_platform]"
    ]
}

Perhaps it would be helpful to have another person look at this and help show me what I'm missing?

@altendky

This comment has been minimized.

Contributor

altendky commented May 5, 2018

Or, maybe you see the issue and the fix but you want it applied as a bigger improvement? This 'method' doesn't use self so it could be moved into a regular file instead of a patch. Perhaps it could also be merged with some other existing function. Am I getting closer?

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 5, 2018

Oh I see. You’re saying this is part of the resolver we already patched? I’m on mobile so not that helpful right now but if this is something we did in the first place I’d be more okay with this. Ultimately I’m just going to want to take a bit of a look at this myself and see if there is a meaningful alternative before I keep saying abstract things. This might be right.

@altendky

This comment has been minimized.

Contributor

altendky commented May 9, 2018

dist.extra = finder.get_extras_links(

Looks like a typo as I can't find any other relevant reference to .extra. Only this on PackageFinder. Of course, please do double check in case I am searching wrong.

self.extra = None

There is .extras on the object, but it's a read-only attribute. inspect.getmodule(dist) seems to say this is from here.

class Distribution(object):

And the extras property.

@property
def extras(self):
return [dep for dep in self._dep_map if dep]

But back to the typo. It's not just that since assigning to .extras obviously wouldn't work. I'll poke around a little bit more and see what I can learn.

altendky added some commits May 9, 2018

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 16, 2018

Sorry for the suuuuper long delay on this, I'll be able to take a look at it this week with luck. I'm just dreading it because it is legitimately complex

@techalchemy

This comment has been minimized.

Member

techalchemy commented May 17, 2018

Ok @altendky I did some testing on this and I think it actually is good to go, sorry for the back and forth and thanks a ton for all your effort tracking this down.

You'll be interested to know this fixes a very specific bug parsing extras like the ones formatted in the requests setup.py /cc @nateprewitt

altendky added some commits May 17, 2018

@classmethod PackageFinder.get_extras_links() until it moves
Not that I like @classmethod but it is honest here and lets the function be tested directly.
@altendky

This comment has been minimized.

Contributor

altendky commented May 17, 2018

I can't say I carefully designed these tests, but they're something. Since get_extras_links() is presently used in both patched/notpip and patched/piptools I wasn't really sure where to move it to. Perhaps when the notpip usage issue is resolved we can more confidently pick a spot for it, if it needs to move at all.

altendky added some commits May 17, 2018

@techalchemy techalchemy merged commit ce13a9c into pypa:master May 23, 2018

2 checks passed

buildkite/pipenv Build #154 passed (7 minutes, 40 seconds)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@altendky altendky deleted the altendky:2138-notpip_looping branch May 23, 2018

@techalchemy techalchemy moved this from Done to Needs Changelog in 2018.06.x Release Jun 16, 2018

@techalchemy techalchemy moved this from Needs Changelog to Done in 2018.06.x Release Jun 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment