-
Notifications
You must be signed in to change notification settings - Fork 414
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 --requirement
option for pipx inject
#1037
Conversation
Do we want to allow |
Did you mean something like |
Yes, sorry |
Probably no then, since I guess most users will choose to edit their requirements files directly. |
Seems like it's impossible to do that currently. I've added a check and it will raise an exception if |
Probably need to mention this exclusiveness in the flag help string. Otherwise looks good to me. |
if requirement_files: | ||
packages_list = [] | ||
for file in requirement_files: | ||
with open(Path(file), "r") as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of reading the files, should we just forward the path to pip to let it parse instead? Requirements files are technically a pip-specific format and parsing them ourselves seems like a big future source of code bloat down the road. Instead we should just redirect all the responsibilities to pip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the question is that how should we determine package names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the package names? This is inject
not install
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we need to forward those package names to pipx_metadata
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be viable to do a pip freeze
before and after installation and record the diff?
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it looks like the installation report will also report the dependencies of a package, which are not specified in requirements.txt. Though we can exclude them from pipx_metadata by checking whether they are in the
requires_dist
field, I personally think this is too complicated
There’s a field requested
for this, we can simply filter out entries with this set to true (meaning it’s from user input i.e. the requirements file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, didn't notice that, thanks for pointing it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for the report to include options passed to pip (such as --pre
, --index-url
) in the requirements file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think either is strictly needed since pipx would install those packages with the exact URL (which the report contains), and the flag is irrelevant in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge conflicts need fixed.
Seems stalled, we can reopen if you pick it up again. |
I've made a new PR #1252 for this, based partially on this one. |
docs/changelog.md
Summary of changes
Add
--requirement
option forpipx inject
.Close #934
Test plan
Tested by running