Skip to content

Allow for specifying local wheels and sdists as dependencies #215

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

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Jun 13, 2022

It is useful, especially when creating custom versions of dependencies that are compatible with CPython main, to specify local file paths as dependencies. Unfortunately, that's blocked by a feature that treats any dependency that matches a file path as a requirements file. This makes that check a little more specific which allows for installing wheels.

This feels like the "safe" backward compatible change, but to be honest, I don't know why this feature exists in the first place -- it doesn't seem to be documented and the unit tests don't fail when you remove this whole if clause. I would also be ok with just removing this if clause if preferred.

@mdboom mdboom force-pushed the requirements-files branch from a352d6f to 2d59043 Compare June 13, 2022 14:47
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM, Could you add a test for this? I think a simple test with mock would suffice. Thanks!

@mdboom mdboom requested a review from kumaraditya303 July 12, 2022 15:30
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

For the most part this seems good. There is basically just one thing that's throwing me off.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

@mdboom mdboom force-pushed the requirements-files branch from 197623f to 88ce6f5 Compare July 27, 2022 15:24
@mdboom mdboom requested a review from ericsnowcurrently July 27, 2022 15:24
@mdboom
Copy link
Contributor Author

mdboom commented Jul 27, 2022

Had to go back to the drawing board on this approach, since install_requirements doesn't actually have any context to know the directory of the requirements.txt file. I instead moved the resolving to a full path to _add_from_file which parses the requirements.txt file.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

(and thanks for adding good tests)

@ericsnowcurrently ericsnowcurrently merged commit 572cb6d into python:main Jul 27, 2022
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.

4 participants