Support pip_args for shared_libs-enabled virtual environments

[Arpafaucon]

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

Summary of changes

Fix #964

My diagnostic of the issue is:

• the shared_libs feature creates a verification task at each Venv instance creation that checks if shared libraries should be updated
• this verification task can decide to run pip commands, and if so, we'd like to have it respect user-provided pip_args options
• the problem: sometimes Venv instances are created in a context where the user does not expect pip to run. An example of this is Option for the list command not to go out to the internet #1081
  • the list command creates Venv instances, those run the shared_lib verification
  • the list CLI options do not enable --pip_args (and IMHO even if it did, users would not expect those here)

Change summary

• move the shared lib verification to a dedicated method Venv. check_upgrade_shared_libs
• call this method only in contexts where a user would expect pip install/upgrade operations (we can identify these because we have pip_args available), just after the Venv instance creation
• thus there will be some places where the shared lib check will not be run anymore
  • notably during pipx list, so --skip-maintenance is a no-op

Test plan

Tested as part of the main test suite. Added tests

• test_install.py::test_pip_args_forwarded_to_shared_libs
• test_reinstall_all.py::test_reinstall_all_triggers_shared_libs_upgrade
• test_run.py::test_pip_args_forwarded_to_shared_libs
• in test_list.py, renamed test_skip_maintenance->test_list_does_not_trigger_maintenance

[chrysle]

That was quick! I generally agree with the behaviour change. You'll need to create a functional test for the behaviour.

[chrysle]

• notably during pipx list, so --skip-maintenance is a no-op

cc @Gitznik

[Arpafaucon]

That was quick! I generally agree with the behaviour change. You'll need to create a functional test for the behaviour.

Could you help me with some pointers for that ? I don't know how to properly test that we actually forwarded the correct pip-args all the way down

[chrysle]

I don't know how to properly test that we actually forwarded the correct pip-args all the way down

You could parse the verbose output?

[Arpafaucon]

I don't know how to properly test that we actually forwarded the correct pip-args all the way down

You could parse the verbose output?

Thanks: I added a test test_install.py::test_pip_args_forwarded_to_shared_libs that checks that providing --no-index to pipx install will effectively break the shared lib update. This is kind of indirect, but has the benefit of not needing any network - and the breakage is easy to test.

If the general principle is OK for you, I can work on adding the same concept to other affected pipx commands

• upgrade
• run
• inject
• reinstall

Let me know if that fits your expectations, or if you have further advice for me

[Arpafaucon]

I don't know how to properly test that we actually forwarded the correct pip-args all the way down

You could parse the verbose output?

Thanks: I added a test test_install.py::test_pip_args_forwarded_to_shared_libs that checks that providing --no-index to pipx install will effectively break the shared lib update. This is kind of indirect, but has the benefit of not needing any network - and the breakage is easy to test.

If the general principle is OK for you, I can work on adding the same concept to other affected pipx commands

* upgrade

* run

* inject

* reinstall

Let me know if that fits your expectations, or if you have further advice for me

Mmm my test methodology stops working at soon as multiple tests are executed. It looks like the shared libs mecanism is shared across tests ? If you have any advice on this that'd be helpful, on my side I'll keep on experimenting

[Gitznik]

• notably during pipx list, so --skip-maintenance is a no-op

cc @Gitznik

Thanks for the ping. I don't mind this change. Is there any clear benefit of upgrading shared packages unless you want to use them (e.g. install, ...)?

If the --skip-maintenance flag is a no-op after this change we should log a deprecation warning and plan on removing the flag in an upcoming release.

[Arpafaucon]

Hey - getting back after a brief gap. I'm still up to carry this change and get to a change that would satisfy pipx team.

I've seen you added the 'awaiting response' badge so I guess there is a misunderstanding - I was myself waiting for answers from you ^^ To clarify and unblock the discussion:

• regarding the maintenance operation during pipx list

  • (A) do we add a --pip-args to enable user customization
  • (B) or do we avoid any maintenance there? If so, we should get rid of the --skip-maintenance flag as suggested by @Gitznik ?

  My humble opinion is (B): I wouldn't expect any maintenance task to happen during a list operation.

• regarding the test strategy, if the sample test looks like something you'd accept: I'll keep experimenting. I had stopped working on it awaiting feedback - but I will take the absence of criticism as a positive answer and carry on :)

[chrysle]

My humble opinion is (B): I wouldn't expect any maintenance task to happen during a list operation.

Let's make it so, then.

[Arpafaucon]

Thanks for the clarifications, I reworked on this to polish it

skip-maintenance CLI argument
I changed the CLI to make --skip-maintenance a no-op. I haven't found guidelines on how to implement deprecations here, nor an example of how it was done in the past, so I improvised a bit:

• if the flag is enabled, I added a logger.warning message
• I updated the doc to mention it is now a no-op
• the commands.list function does not take it as an argument anymore
• Should I also raise a DeprecationWarning ? With its current configuration, it seems that pipx does not show them, I could try and fix that

tests
I managed to fix the test by using a better fixture, now the whole test suite for 3.10 works on my machine - I'll check the full CI once you approve the workflow. I added the same for pipx run because that was trivial, but I am unsure whether it's useful to work more to test the same behaviour for the other commands

[Gitznik]

skip-maintenance CLI argument I changed the CLI to make --skip-maintenance a no-op. I haven't found guidelines on how to implement deprecations here, nor an example of how it was done in the past, so I improvised a bit:

• if the flag is enabled, I added a logger.warning message
• I updated the doc to mention it is now a no-op
• the commands.list function does not take it as an argument anymore
• Should I also raise a DeprecationWarning ? With its current configuration, it seems that pipx does not show them, I could try and fix that

I don't think we have to dive deeper into deprecation warnings, as #1278 plans to introduce a global flag for --skip-maintenance, which will reintroduce it (even though it's a no-op for list).

[Arpafaucon]

I'm having trouble making the tests on windows pass - to be fully honest I don't understand the root cause of why this platform behaves differently from ubuntu+macOs (but hey, windows...), but it seems I've found a way for it to work by changing --pip-args='--no-index' to --pip-args=--no-index.
Sorry for the multiple CI runs, I don't have another way to test on windows ATM

[Gitznik]

I'm having trouble making the tests on windows pass - to be fully honest I don't understand the root cause of why this platform behaves differently from ubuntu+macOs (but hey, windows...)

We've all been there

Sorry for the multiple CI runs, I don't have another way to test on windows ATM

You can also enable workflows on your fork so you don't have to wait for us approving each run :)

[christian-krieg]

Hey there, first of all, thanks for this fix and great conversation so far! As I am waiting for this PR to be closed in order to start working on #1278, I would like to kindly ask when I may expect the merge? :)

[Gitznik]

@chrysle , @Arpafaucon I actually don't think there's anything missing?

[Arpafaucon]

@chrysle , @Arpafaucon I actually don't think there's anything missing?

AFAIK not on my side, I'm ready for merge if you guys are. There are 2 points by @chrysle waiting to be resolved

• the first one on changelogs should be good
• there was also a discussion about explicit-vs-implicit default value for pip_args. This PR leans strongly on the explicit side (pip_args: List[str]), but I'm not certain this fits @chrysle 's vision as he had the opposite suggestion (I remember he suggested pip_args: Optional[List[str]] = None, but I can't see the comment anymore). This was some time ago so maybe the current state is now OK

[chrysle]

This PR leans strongly on the explicit side (pip_args: List[str]), but I'm not certain this fits @chrysle 's vision as he had the opposite suggestion

I added a comment on the general issue at #1256 (comment). This LGTM now, thanks for the work you invested into this piece!