Skip to content
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

Feature/better resolver #2267

Merged
merged 18 commits into from
May 28, 2018
Merged

Feature/better resolver #2267

merged 18 commits into from
May 28, 2018

Conversation

techalchemy
Copy link
Member

Patch piptools to use current environment python

Signed-off-by: Dan Ryan <dan@danryan.co>
- Fixes #2088, #2234, #1901
- Fully leverage piptools' compile functionality by using constraints
  in the same `RequirementSet` during resolution
- Use `PIP_PYTHON_PATH` for compatibility check to filter out
  `requires_python` markers
- Fix vcs resolution
- Update JSON API endpoints
- Enhance resolution for editable dependencies
- Minor fix for adding packages to pipfiles

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
except TypeError:
ireq.version = dist.version
ireq.project_name = dist.project_name
ireq.req = dist.as_requirement()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t quite understand why these are needed :|

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just have to believe me that they are, based on errors I encountered

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes the InstallRequirement doesn't properly get these values set on it during the resolution process because of ephemeral wheel building moving on too quickly or something along those lines, it's quite hard to pin down exactly where in the stack it is going wrong which is why I added an additional safeguard to try and get the actual name of the package in question and resolve the actual requirement and version -- these elements are critical for downstream resolution if we want to provide real conflict resolution

This shouldn't affect any functionalities, only clean up some generator
usages to reduce repeated looping through requirements.
pipenv/utils.py Outdated
if ' -i ' in dep:
dep, url = dep.split(' -i ')
req = Requirement.from_line(dep)
constraints.append(req.as_line())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this from_lineto_line combination do? Can’t dep be added directly into constraints?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short answer is no, the long answer is 'a lot of things happen here'. The line we pass in could be anything, but the intermediary step ensures that, for example, if it is a local path, we tell pip to name it properly, or if it is a wheel, we do the same. This step also ensures that we format the line in the proper filesystem-appropriate format, we supply the formatted markers correctly, it essentially offers us an extra layer of sanity checking and validation. At some point we can stop passing 'deps' around at all and just pass requirement objects, I just haven't done the code cleanup for that.

release_requires = self.session.get(release_url)
for requires in release_requires.json().get('info', {}).get('requires_dist', {}):
i = InstallRequirement.from_line(requires)
if self.DEFAULT_INDEX_URL not in self.finder.index_urls:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for cleaning this garbage up, seriously this was so horrible to look at

techalchemy and others added 10 commits May 27, 2018 02:23
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
 - The specifierset will fail otherwise
 - We don't want to include only things that have this
   -- we want to _exclude_ things that do not match it

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
- fix for windows vcs urls

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
- convert to string properly
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
@uranusjr uranusjr merged commit e135f7c into master May 28, 2018
@techalchemy techalchemy deleted the feature/better-resolver branch June 8, 2018 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipenv installs incorrect version of package
2 participants