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

Allow installing multiple applications at once #1102

Merged
merged 13 commits into from
Dec 18, 2023

Conversation

sam-sw
Copy link
Contributor

@sam-sw sam-sw commented Nov 4, 2023

  • I have added an entry to docs/changelog.md

Summary of changes

As a response to issue #971, the commands.install function is modified to take lists of package names/specs as arguments, and iterate through the lists, installing each application separately. The associated CLI functions and help strings are updated to reflect this. The commands.reinstall function is also modified to ensure it complies with the new argument requirements of commands.install. Two tests have been added (replicating test_install_easy_packages and test_install_tricky_packages, but doing multiple packages at once).

Test plan

Tested by manually running

pipx install pycowsay black

and also with the new unit tests.

Resolves #971

@sam-sw
Copy link
Contributor Author

sam-sw commented Nov 4, 2023

This is my take on responding to #971, happy to receive comments if a different approach may work better.

Copy link
Contributor

@chrysle chrysle left a comment

Choose a reason for hiding this comment

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

This is an approach for creating a separate virtual environment for each of the packages.
Should an option be added to install all into the same one?

Also, I'd think one should also be able to remove and upgrade multiple packages. But that's out of the scope of this PR.

src/pipx/main.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@sam-sw
Copy link
Contributor Author

sam-sw commented Nov 6, 2023

I agree with your comment about removing and upgrading multiple packages.

For installing multiple packages to the same virtual environment, would the current approach be to install one, and then inject the others, using the --include-apps option to ensure all applications are on the PATH? This is definitely more involved for users than a separate option on the install command, but then again the main reason for using pipx is to separate applications into unique virtual environments.

Personally I would lean away from having the extra install option, to keep things simple.

@uranusjr
Copy link
Member

Should an option be added to install all into the same one?

This is already avaiable with --preinstall. It makes more sense to have --postinstall if needed.

@chrysle
Copy link
Contributor

chrysle commented Nov 22, 2023

This is already avaiable with --preinstall.

Though this doesn't invite you to install multiple packages at once and doesn't make them globally available.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Needs merge conflicts fixed.

@gaborbernat gaborbernat marked this pull request as draft December 2, 2023 05:44
@gaborbernat
Copy link
Contributor

Seems stalled, we can reopen if you pick it up again.

@gaborbernat gaborbernat closed this Dec 8, 2023
Fix conflicts and formatting.
@sam-sw
Copy link
Contributor Author

sam-sw commented Dec 15, 2023

Apologies for the delay, merged latest changes from the main branch now and fixed any conflicts etc.

@gaborbernat
Copy link
Contributor

Seems you still need to fix some tests. 🤔

@sam-sw
Copy link
Contributor Author

sam-sw commented Dec 15, 2023

I've just re-ran nox locally and tests seem to be passing, except Python 3.9. But that also fails on the main branch as well?

@gaborbernat gaborbernat reopened this Dec 15, 2023
@gaborbernat
Copy link
Contributor

@gaborbernat
Copy link
Contributor

Push a new commit to trigger a new CI run.

@sam-sw
Copy link
Contributor Author

sam-sw commented Dec 16, 2023

Looks like 3.8 and 3.9 fail because the strict=True argument in zip isn't implemented for those versions. Using zip this way was to satisfy the pre-commit check for code linting, what would you recommend?

@chrysle chrysle marked this pull request as ready for review December 18, 2023 15:05
tests/test_install.py Outdated Show resolved Hide resolved
Copy link
Contributor

@chrysle chrysle left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat merged commit 6cccbf8 into pypa:main Dec 18, 2023
14 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.

Allow installing multiple applications with install command
4 participants