pipenv icon indicating copy to clipboard operation
pipenv copied to clipboard

Issue 4371 incorrect dependencies when install dev packages

Open dqkqd opened this issue 3 years ago • 1 comments

The issue

I'm trying to fix https://github.com/pypa/pipenv/issues/4371. I think it's better to consider 2 cases below:

  • Installing from Pipfile.
  • Installing from provided packages (the issue itself).

The fix

Installing from Pipfile

I read through the code, and here is what it does when installing packages from Pipfile:

  1. Resolve dev-packages dependencies. Write to lockfile as default.
  2. Resolve packages dependencies. Write to lockfile as develop.
  3. Overwrite duplicated develop packages by default packages.
  4. Install default and develop packages in the lockfile.

Because, dev-packages is resolved separately, there might be a chance where dev-packages is overwritten but its dependencies are not (they aren't duplicated with packages). So I think the result dependency graph would be more accurate if packages is considered as constraint while resolving dev-packages dependencies. Luckily pip provided constraints files.

I modified the code to do the followings:

  1. Resolve packages dependencies. Write to lockfile.
  2. Create constraints file from packages. Skip editable and extras packages as describe in pip constraints files.
  3. Resolve dev-packages dependencies using constraints file above. Write to lockfile as default.
  4. Overwrite duplicated packages. Although dev-packages was resolved with constraints, this step is also important because there some cases constraints might be skipped (editable and extras packages).
  5. Install resolved packages.

Installing from provided packages

When packages are passed as arguments, pipenv does the followings:

  1. Install passed packages without resolving dependencies. Write to Pipfile and lockfile.
  2. Resolving dev-packages and packages independently. Modify the lockfile.
  3. Overwrite duplicated packages.
  4. Install all the packages in the lockfile again. There might be a case it will uninstall the packages from step 1 before installing.

Similar to the first problem, I tried to use default packages as constraints to resolve packages dependencies. I think it is enough to just resolve dev-packages at step 1. Because default packages should be installed directly, and resolved packages at step 2 don't need to be resolved again.

The checklist

  • [ ] Associated issue
  • [ ] A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

dqkqd avatar Aug 08 '22 18:08 dqkqd

@dqkqd I left a few more notes about areas I think we could further refine the code and reduce duplication. I think if we can factor out the three spots we write constraint files into a common utility and some of the places we can use @cached_property to allow for simpler and more readable properties. I think the change makes sense and could be merged, but chipping away at the some more of this type of technical debt would be good.

matteius avatar Aug 10 '22 03:08 matteius

@dqkqd Nice work!

matteius avatar Aug 13 '22 09:08 matteius

@matteius Thank you.

dqkqd avatar Aug 13 '22 09:08 dqkqd