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

Add nested vcs dependency subdirectory when locking to Pipfile.lock (Fix #6120) #6136

Merged

Conversation

AlexandreArpin
Copy link
Contributor

@AlexandreArpin AlexandreArpin commented Apr 22, 2024

The issue

When installing a package that has a vcs dependency with a subdirectory, the subdirectory isn't added to the Pipfile.lock.

This is a tentative fix for #6120

The fix

When formatting the vcs requirement for lockfile in format_requirement_for_lockfile

  • normalize_vcs_url to clean the vcs's url as is done in install_req_from_pipfile - this removes the fragments from the url
  • Inspect and add the req.link.subdirectory_fragment to the lockfile if parsed by the Link object

As discussed in #6120, alternatives could have been to honor #4259 and work on the install portion to be able to resolve correctly the Pipfile entry with the subdirectory as a vcs url fragment.

The checklist

  • Associated issue
  • A news fragment in the 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 #.

@AlexandreArpin AlexandreArpin force-pushed the hotfix/nested-vcs-dependency-lock branch from 60a7c83 to 0bdb85d Compare April 22, 2024 02:20
@AlexandreArpin
Copy link
Contributor Author

From: #6120 (comment)

@AlexandreArpin your patch sounds like the right direction to head in -- I'm getting lock resolution errors though with the egg fragment syntax, does your patch resolve for that as well?

There was some discussion in #6095 about how the egg fragment syntax was deprecated?

I opted not to parse it and add it in the patch with that context.

I'd be happy to look into fixing the issue you mentioned in your comment if you'd like :)

Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

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

Looks good.

@matteius matteius merged commit 584e917 into pypa:main Apr 23, 2024
19 checks passed
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.

None yet

3 participants