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

Resolve dependencies of local wheels #1944

Merged
merged 4 commits into from
Apr 11, 2018
Merged

Conversation

techalchemy
Copy link
Member

Signed-off-by: Dan Ryan dan@danryan.co

@techalchemy
Copy link
Member Author

@vphilippon @uranusjr @jtratner @ncoghlan would one of you mind taking a look at this (preferrably one of you + @vphilippon just in case this is a terrible idea)

- Fixes #1937, #1683
- Modifies the resolved wheel info on the way into the lockfile to
  retain the relative path

Signed-off-by: Dan Ryan <dan@danryan.co>
@@ -1128,6 +1128,12 @@ def do_lock(
# Add default dependencies to lockfile.
for dep in results:
# Add version information to lockfile.
pipfile_version = project.packages[dep['name']] if dep['name'] in project.packages else None
Copy link
Member

Choose a reason for hiding this comment

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

Does project.packages.get(dep['name']) work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes despite being a toml object this works fine

@@ -120,7 +120,7 @@ def resolve(self, max_rounds=12):
@staticmethod
def check_constraints(constraints):
for constraint in constraints:
if constraint.link is not None and not constraint.editable:
if constraint.link is not None and not constraint.editable and not (constraint.is_wheel or constraint.is_artifact):
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 is_artifact add to this check? (Asking because the title says this resolves wheels.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes. I set this up because I wasn’t sure if we wanted to resolve artifacts too. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Doing that sounds plausible, but it would probably make sense to pursue it as a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed and pushed, will let it build and then merge

Copy link
Member

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

I have the same questions as @uranusjr, but otherwise this looks good to me :)

…lve-wheels

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
@techalchemy
Copy link
Member Author

Hmm are we on a minor version bump now?

@andyneff
Copy link

How much harder would it be to make this work on zip and tarball urls like in #772?

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 does not lock dependencies of installed wheels
4 participants