-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Pipenv lock fails for packages with extras as of 2022.8.13 #5273
Comments
@dqkqd fyi -- do you have some cycles to help me think more about this at it relates to the constraint files work? |
@matteius Yes, there's a bug here. pipenv/pipenv/utils/dependencies.py Lines 287 to 290 in 697f1a9
I will open a PR for this. |
Function to check constraints from pipenv/pipenv/patched/pip/_internal/req/req_install.py Lines 854 to 880 in 697f1a9
While pipenv/pipenv/utils/dependencies.py Lines 287 to 298 in 697f1a9
|
This is fixed in |
Thankyou guys for the extremely quick turnaround on this. |
In
And failures occurring if a file was used to install a package (which from what I can tell, is still supported). |
@antoniomika Is the file nor marked editable = "true" in the @dqkqd Could you look into that |
Here's the stacktrace we were seeing:
We don't have
from here. Let me know what else I can do to help! |
I see, function pipenv/pipenv/utils/dependencies.py Lines 281 to 283 in 0d86629
That pipenv/pipenv/patched/pip/_internal/req/req_install.py Lines 854 to 880 in 0d86629
Rewrite that function is a solution but we have to keep track if pip changes it as well. Is there anyway we could use it without emitting any warning? |
Unnamed reqs (files) and editable ones might be a different story. |
I don't think we need to rewrite the pip function, I think its actually warning us of valid errors. I think the issue is that we need to filter out certain requirements from the
|
Because all of the packages from |
@dqkqd I see what you mean now, we are using that method to filter them out already, but thats causing the emitting of a deprecation warning because that method seems to expect that data was already cleaned -- probably we should just have our own method for this like you were suggesting, be it a similar copy or a simplified refactor. That begs the question of what is causing resolution failures for @antoniomika -- can you elaborate on the file configuration setting you are using? Do you just mean local editable installs, or something else? |
It was a package fork with some additions from a PR. Here's the line:
|
@antoniomika It would appear you must have a conflicting requirement with something that specific zip requires, because I can install it with requests by themselves: Have you tried running
|
@matteius interesting. I went down that route and couldn't find anything specific. I also found I'll keep digging, but at the least the deprecation fix would still be a nice to have. |
@antoniomika are you locking dev dependencies? |
We have both. I'm running a pipenv update. |
@antoniomika There may be a package shared between default and dev that are on conflicting versions? |
@matteius not that I know of, but I'm doing a deeper dive now. I have confirmed everything works fine with the older version but I'll update you once I know more. |
@antoniomika if you see two package with the same name in the |
@matteius I was able to find the constraint that was the issue, there were two different versions. Is it possible to get that information outputted as an error? I'm now seeing this issue with a different package:
I outputted the actual constraint and it doesn't look like a name is associated with it (the output is above). Not sure if that's something that needs to be fixed or is another problem on our end (which I assume it is?). |
That looks like a possible bug, somehow the Install requirement name is None. I'll think more about it. Cc @dqkqd |
Yes, it's from the commit d33f4ec. self._constraints.sort(key=lambda ireq: ireq.name or "") At first, I thought package name is unique. Turn out I was wrong, when there are multiple |
Does making the name the url not solve the issue as well (and removes the duplicate issue)? That wouldn't necessarily solve all cases though. |
@dqkqd we had a report that the constraints issue was not solved even with your change: #4660 (comment) I am going to open a PR to revert this since based on the prior report: https://github.com/pypa/pipenv/pull/5299/files I also did some sorting in this PR yesterday but I think these are only pip constraints that get passed this way: https://github.com/pypa/pipenv/pull/5313/files#diff-3542045ed7f41738cd4519e02753c9e83af5654c56c124598f0fbbd82405ec49R134 |
Well reverting the commit @dqkqd made does cause the constraints to jump around a lot more when locking, probably we need to think through this more than a revert. I like the idea of making it safer, but defaulting to empty string seems problematic. |
I knew the root of this issue now, sorting or not isn't the problem. It's the constraints with |
(for reference, the fork I had was unneeded and I removed that dependency in favor of the upstream version. The latest issue I had was with |
@dqkqd Is it the is_constraints method do you think? If so I suspect we could also check if the InstallRequirement does not have a name in your |
I think it's the bug in pipenv/pipenv/utils/dependencies.py Lines 274 to 286 in f04071c
For example: But the actual output is The name is disappeared because: pipenv/pipenv/vendor/requirementslib/models/requirements.py Lines 2801 to 2805 in f04071c
pipenv/pipenv/vendor/requirementslib/models/requirements.py Lines 219 to 220 in f04071c
And because it has url, so it's name is discarded from now. pipenv/pipenv/vendor/requirementslib/models/requirements.py Lines 753 to 760 in f04071c
|
Or maybe I'm wrong, new_dep = Requirement.from_pipfile(dep_name, dep)
if new_dep.is_named and is_constraints(new_dep.as_ireq()):
c = new_dep.as_line().strip()
constraints.append(c) But the documentation does not say that. So maybe it could have url as constraints? I tried using this constraints file with # constraints file
django-redis @ https://github.com/antoniomika/django-redis/archive/4.12.2.zip |
@dqkqd I found this interesting and has a lot of context: pypa/pip#8210 That was a long read, but the conclusion is that "unnamed constraints are not allowed." |
Issue description
There was a regression in the ability to lock Pipfiles containing packages with extras on the 2022.8.13 release. This was working fine on versions prior to 2022.8.13. I believe this is as a result of the changes introduced in #5234
Expected result
Given the Pipfile in the replicate section, it should lock, showing
Actual result
Steps to replicate
With the following Pipfile
Run
pipenv lock
Note that there is a package with 'extras', namely uvicorn[standard]. This issue does not occur with normal packages.
The change to lock the packages first and then treat that lock as constraints for the dev packages seems to raise the exception that the extras are not allowed.
The text was updated successfully, but these errors were encountered: