Inject additional packages from text file

[jamesmyatt]

• I have added a news fragment under changelog.d/ (if the patch affects the end users)

Summary of changes

Fixes #934. Provides for injecting dependencies from a text (requirements) file.

Replaces #1037, which it's partially based on. Thanks, @dukecat0!

I don't think it's necessary to match the pip functionality when the "runpip" command also exists. Instead it replaces the workaround with xargs:

cat <requirements-file> | sed -e 's/#.*//' | xargs pipx inject <package>

It could be extended to support the full pip-requirements syntax in the future, if necessary, but I'd prefer "good" rather than "perfect".

Test plan

Tested by running unit tests

[chrysle]

Thank you for working on this! I'm a bit unsure regarding the naming, though, since "Requirements File" is a reserved term.

[Gitznik]

Also flagging that in the original PR by @dukecat0 the decision was made to make the requirements file and passing a requirement directly mutually exclusive.

I personally don't see a technical reason to stick to that, so not a blocker for me.

[jamesmyatt]

Also flagging that in the original PR by @dukecat0 the decision was made to make the requirements file and passing a requirement directly mutually exclusive.

I personally don't see a technical reason to stick to that, so not a blocker for me.

I think that's how pip works, but like you say, I don't see a good technical reason for it. I don't even think it's necessary to match the pip functionality. If you want to match the pip functionality, then runpip is there. My preference here is to be fairly naive and rely on the users to do sensible things.

[jamesmyatt]

Thank you for working on this! I'm a bit unsure regarding the naming, though, since "Requirements File" is a reserved term.

I agree that "Requirements File" is a reserved term and shouldn't be over-used. However, we're using a strict sub-set of the "Requirements File" syntax, won't accept changes that are incompatible and have ambitions to support the full syntax eventually.

edit: To summarise, I think it's better to describe it as "partial pip-requirements-file support" rather than "not a pip-requirements file".

[jamesmyatt]

On the subject of full pip-requirements file support. I wonder if the better long-term implementation would be to change the way that inject works to just create a temporary requirements file from the CLI inputs and then install that using pip (I think this is what conda does with pip requirements). Then pipx would record everything that's listed as requested in the installation report (based on #1037 (comment)).

But I think that needs more investigation (e.g. checking how it works with upgrade --include-injected) and would be a much more significant change to pipx. Hence, my preference is to implement this solution with partial pip-requirements-file support that is probably good enough for 99% of use cases, and investigate a solution for the remaining 1% in slower time.

[chrysle]

But I think that needs more investigation (e.g. checking how it works with upgrade --include-injected) and would be a much more significant change to pipx. Hence, my preference is to implement this solution with partial pip-requirements-file support that is probably good enough for 99% of use cases, and investigate a solution for the remaining 1% in slower time.

This sounds like a good plan to me.

[chrysle]

@jamesmyatt Gentle ping on this.

[jamesmyatt]

Thanks for the reminder. I think the outstanding actions are:

• Fix merge
• Check docs are clear about relationship to pip-requirements files
• Add examples to documentation
• Check log and std outputs in unit tests

Anything else?

[chrysle]

Anything else?

I don't think so.

[huxuan]

@chrysle Do you have an idea why the new unit tests are sometimes failing?

isort is one of the dependencies of pylint, so you may need to change the test case.

[chrysle]

isort is one of the dependencies of pylint, so you may need to change the test case.

I'd suggest to just drop pylint as a test package in this case.

[jamesmyatt]

@chrysle Do you have an idea why the new unit tests are sometimes failing?

isort is one of the dependencies of pylint, so you may need to change the test case.

Ah yes. I wonder why it works sometimes then.

[jamesmyatt]

I think this is actually done now

[chrysle]

Waiting for @Gitznik's review before merging.

[Gitznik]

LGTM aside from the news fragment 👍

[huxuan]

Great job!