Fixed a bug with locking packages with non canonical names #6057
Conversation
|
From my point of view the patch can't be send to pip. |
|
This seems like a legitimate fix, yet ... patching pip is a cause of trouble. like other pip patches. If @matteius is OK with this patch I can move it there. |
|
No problem i would move the fix there. |
|
Please go a head and move the patch there. You will also need to apply it to check that our vendoring system understands it. |
|
I am convinced this must be fixable in the code we use from pipenv without patching pip at all. I will try to help look more into this tonight, assuming we don't loose power in this storm. |
|
Does this have to do with dash vs underscore in the package name? I do think I broke something there when we factored away requirementslib. |
pipenv/utils/resolver.py
Outdated
| @@ -210,14 +212,14 @@ def create( | |||
| pipfile_entries[package_name] = pipfile_entry | |||
| if isinstance(pipfile_entry, dict): | |||
| if packages[package_name].get("index"): | |||
| index_lookup[package_name] = packages[package_name].get("index") | |||
| index_lookup[canonical_package_name] = packages[package_name].get("index") | |||
There was a problem hiding this comment.
Let's create mapping with canonical package names instead of raw package names
| @@ -25,6 +25,7 @@ | |||
| from pipenv.patched.pip._internal.req.req_install import InstallRequirement | |||
| from pipenv.patched.pip._internal.utils.temp_dir import global_tempdir_manager | |||
| from pipenv.patched.pip._vendor import pkg_resources, rich | |||
| from pipenv.patched.pip._vendor.packaging.utils import canonicalize_name | |||
| from pipenv.project import Project | |||
| from pipenv.utils.fileutils import create_tracked_tempdir | |||
| from pipenv.utils.requirements import normalize_name | |||
There was a problem hiding this comment.
From my point of vie we should use canonicalize_name funiction from pip instead of normalize_name function everywhere.
WDYT?
| else: | ||
| index_lookup[package_name] = project.get_default_index()["name"] | ||
| index_lookup[canonical_package_name] = project.get_default_index()["name"] | ||
| if install_req.markers: | ||
| markers_lookup[package_name] = install_req.markers |
There was a problem hiding this comment.
Maybe we should use everywhere canonical_package_name instead of raw_package_name.
But i didn't start to do it because it would make affect on locking files => i'm a little bit afraid of it.
There was a problem hiding this comment.
Yeah you are right, I wish this is something I hadn't broke, but I think your patch makes sense as an iteration in the right direction. I will think more about it.
There was a problem hiding this comment.
Can we apply my patch?
P.S. I sent it because at my team we have some issues with pipenv. And we can't migrate to the freshest version (we still use pretty old one)
|
@matteius can you please take a look one more time? |
mangin
force-pushed
the
bug-locking-uncanonical-packages
branch
from
62f61be to
bc57956
Compare
|
Fixed code style... i forgot to set up precommit hook. Sorry. |
|
Hey @matteius, |
|
@mangin I think we will get another 2023 release in (minior or patch) release within the next week and then start looking at our more major 2024 version release after that. At least that's my plan, I have to travel for work next week so if I can get it in before then,. Our goal is to adopt semver going forward. |
|
@mangin Your patch is in the release from 15 minutes ago -- I hope that it is helpful. |
|
Thank you for the release ^_^ |
The issue
Fix a bug with locking projects that contains packages with non canonical names from private indexes
More details here:
#6056
The checklist
news/directory to describe this fix with the extension.bugfix.rst,.feature.rst,.behavior.rst,.doc.rst..vendor.rst. or.trivial.rst(this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.