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

Conversation

Projects
2 participants
@techalchemy
Member

techalchemy commented May 26, 2018

Patch piptools to use current environment python

  • Fixes #2088, #2234, and #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

@techalchemy techalchemy force-pushed the feature/better-resolver branch from f3a3c56 to 3df97c3 May 26, 2018

techalchemy added some commits May 25, 2018

Use piptools constraints files more properly
Signed-off-by: Dan Ryan <dan@danryan.co>
Patch piptools to use current environment python
- 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>
Fix marker parsing bug
Signed-off-by: Dan Ryan <dan@danryan.co>
Use pipenv cache dir for caching wheels
Signed-off-by: Dan Ryan <dan@danryan.co>

@techalchemy techalchemy force-pushed the feature/better-resolver branch from 3df97c3 to f532b2e May 27, 2018

except TypeError:
ireq.version = dist.version
ireq.project_name = dist.project_name
ireq.req = dist.as_requirement()

This comment has been minimized.

@uranusjr

uranusjr May 27, 2018

Member

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

This comment has been minimized.

@techalchemy

techalchemy May 27, 2018

Member

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

This comment has been minimized.

@techalchemy

techalchemy May 27, 2018

Member

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

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

This comment has been minimized.

@uranusjr

uranusjr May 27, 2018

Member

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

This comment has been minimized.

@techalchemy

techalchemy May 27, 2018

Member

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.

techalchemy added some commits May 27, 2018

Add sources and caches to pip calls
Signed-off-by: Dan Ryan <dan@danryan.co>
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:

This comment has been minimized.

@techalchemy

techalchemy May 27, 2018

Member

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

techalchemy and others added some commits May 27, 2018

Update piptools patch
Signed-off-by: Dan Ryan <dan@danryan.co>
Probably write unicode strings...
Signed-off-by: Dan Ryan <dan@danryan.co>
It is important to check for `python_requires`
 - 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>
Logic fix
Signed-off-by: Dan Ryan <dan@danryan.co>
SpecifierSet logic fix
Signed-off-by: Dan Ryan <dan@danryan.co>
formatting cleanup and posix style paths
- fix for windows vcs urls

Signed-off-by: Dan Ryan <dan@danryan.co>
Use filesystem compatible encodings for strings
Signed-off-by: Dan Ryan <dan@danryan.co>
Fix lockfile filename parsing
- convert to string properly
Don't shell escape posix-style paths
Signed-off-by: Dan Ryan <dan@danryan.co>

@uranusjr uranusjr force-pushed the feature/better-resolver branch from f1a0d65 to a5e69e4 May 28, 2018

Update patch
Signed-off-by: Dan Ryan <dan@danryan.co>

@uranusjr uranusjr merged commit e135f7c into master May 28, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
buildkite/pipenv Build #211 passed (5 minutes, 35 seconds)
Details

@techalchemy techalchemy deleted the feature/better-resolver branch Jun 8, 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

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

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

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

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

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