Updated PR: Added --global option to perform actions for all users (#754) #1281

Jendker · 2024-03-07T03:36:42Z

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

Summary of changes

Adds a --global option to perform actions for all users. This is a resubmission of PR #1088 by https://github.com/haxwithaxe which I rebased on the latest main.

Test plan

Passes your CI process on github (via your unaltered actions in my fork) apart from MacOS test which needs to be investigated. Help from your side is highly appreciated. Test run from my fork: https://github.com/Jendker/pipx/actions/runs/8182365101

To verify the changes do what they are supposed to

python3 src/pipx --global environment

To test all the relevant commands for regressions.

python3 src/pipx --global ensurepath
sudo python3 src/pipx --global install pycowsay
sudo python3 src/pipx --global reinstall pycowsay
sudo python3 src/pipx --global inject pycowsay black
sudo python3 src/pipx --global uninject pycowsay black
sudo python3 src/pipx --global runpip pycowsay freeze
sudo python3 src/pipx --global list
sudo python3 src/pipx --global uninstall pycowsay
python3 src/pipx environment
python3 src/pipx ensurepath
python3 src/pipx install pycowsay
python3 src/pipx reinstall pycowsay
python3 src/pipx inject pycowsay black
python3 src/pipx uninject pycowsay black
python3 src/pipx runpip pycowsay freeze
python3 src/pipx list
python3 src/pipx uninstall pycowsay

Jendker · 2024-03-07T03:37:38Z

Pinging @Gitznik who offered help in the original PR.

chrysle

Thanks for picking this up! However we'll definitely need to add some more documentation, and extend the "How pipx works" documentation.

tests/test_uninstall.py

tests/test_upgrade.py

tests/test_environment.py

src/pipx/paths.py

src/pipx/main.py

Gitznik

Thanks for picking this up!

tests/test_list.py

tests/test_uninstall.py

Jendker · 2024-03-09T07:33:17Z

Thank you for the review and all helpful comments! I had a look into https://pipx.pypa.io/stable/how-pipx-works/ but I am not sure if I will be able to adjust the doc without blurring everything given my limited knowledge of pipx codebase. Some help here from your side would be highly appreciated!

For now, I have updated the docs in docs/installation.md which already mention the env variables so I could add a concise and (hopefully) understandable note regarding --global flag there.

chrysle · 2024-03-09T07:54:46Z

I had a look into https://pipx.pypa.io/stable/how-pipx-works/ but I am not sure if I will be able to adjust the doc without blurring everything given my limited knowledge of pipx codebase.

Probably adding a bullet point mentioning the --global flag and linking to the new paragraph under Installation (which I think looks good) will be enough. This doc is also missing information on how to override the common paths, so providing the link should serve a dual purpose.

tests/helpers.py

Gitznik

Just docs stuff, otherwise lgtm 👍

docs/installation.md

docs/how-pipx-works.md

docs/installation.md

Jendker · 2024-03-09T18:49:36Z

Is there anything I can do to get the tests to pass on MacOS? I am not sure why I get permission denied accessing /opt/pipx on MacOS but it works fine on Ubuntu.

Gitznik · 2024-03-09T20:48:51Z

Is there anything I can do to get the tests to pass on MacOS? I am not sure why I get permission denied accessing /opt/pipx on MacOS but it works fine on Ubuntu.

/opt does not seem to be a thing on MacOS. It is not even mentioned in their file system docs.

You can use platformdirs site_data_dir to generate the appropriate shared path instead I think. You can use it for both Linux and Mac, staying consistent with the current way of doing things (even though it may have to be changed during #1257).

chrysle · 2024-03-10T07:56:38Z

You can use platformdirs site_data_dir to generate the appropriate shared path instead I think. You can use it for both Linux and Mac, staying consistent with the current way of doing things (even though it may have to be changed during #1257).

That sounds good for MacOS, but I think we should stick to /opt/ on Linux, as I am fairly sure /usr/local/share/ is mostly a location for system administrators to install their own software.

Gitznik · 2024-03-10T10:49:28Z

That sounds good for MacOS, but I think we should stick to /opt/ on Linux, as I am fairly sure /usr/local/share/ is mostly a location for system administrators to install their own software.

There's a lot of discourse around it online (spend about 20 minutes yesterday reading conflicting opinions on that topic). From what I gathered either would be fine, really. BSD derivatives favor usr/local/share (as with MacOS, /opt is not really a thing there) while Linux itself slightly favors /opt.

IMO - stick to the decision platformdirs has made, as we're not just supporting Linux - we're also supporting BSD derivatives.

chrysle · 2024-03-10T13:45:23Z

IMO - stick to the decision platformdirs has made, as we're not just supporting Linux - we're also supporting BSD derivatives.

Good, but if downstream packagers complain because they feel the need to patch this behaviour I'll direct them to you ;-)

Gitznik · 2024-03-10T14:08:11Z

Good, but if downstream packagers complain because they feel the need to patch this behaviour I'll direct them to you ;-)

If you think that's a realistic scenario, let's go for /opt/ on linux, and /usr/local/share/ for Mac & BSD? We currently don't differentiate between those anywhere AFAIK, so some new logic will be required there.

Jendker · 2024-03-10T17:15:56Z

I tested it locally on my Mac machine and writing to '/opt' works both on Intel and ARM Mac but I have to run commands as a root user with sudo. I’d expect the same on Ubuntu where '/opt' is only writeable for root so I’m surprised that the tests are passing on Ubuntu without any sudo calls.

Jendker · 2024-03-10T19:37:25Z

To follow up on my last comment I just run tests in with python docker image (debian based) with a non-root user and I got very similar output like in MacOS tests:

________________________________________________________________________________________ test_fetch_missing_python _________________________________________________________________________________________

src = PosixPath('/tmp/tmp9nfdvti7/download/python'), dst = PosixPath('/opt/pipx/py/3.12'), copy_function = <function copy2 at 0x7f454efcdc60>

    def move(src, dst, copy_function=copy2):
        """Recursively move a file or directory to another location. This is
        similar to the Unix "mv" command. Return the file or directory's
        destination.

        If dst is an existing directory or a symlink to a directory, then src is
        moved inside that directory. The destination path in that directory must
        not already exist.

        If dst already exists but is not a directory, it may be overwritten
        depending on os.rename() semantics.

        If the destination is on our current filesystem, then rename() is used.
        Otherwise, src is copied to the destination and then removed. Symlinks are
        recreated under the new name if os.rename() fails because of cross
        filesystem renames.

        The optional `copy_function` argument is a callable that will be used
        to copy the source or it will be delegated to `copytree`.
        By default, copy2() is used, but any function that supports the same
        signature (like copy()) can be used.

        A lot more could be done here...  A look at a mv.c shows a lot of
        the issues this implementation glosses over.

        """
        sys.audit("shutil.move", src, dst)
        real_dst = dst
        if os.path.isdir(dst):
            if _samefile(src, dst) and not os.path.islink(src):
                # We might be on a case insensitive filesystem,
                # perform the rename anyway.
                os.rename(src, dst)
                return

            # Using _basename instead of os.path.basename is important, as we must
            # ignore any trailing slash to avoid the basename returning ''
            real_dst = os.path.join(dst, _basename(src))

            if os.path.exists(real_dst):
                raise Error("Destination path '%s' already exists" % real_dst)
        try:
>           os.rename(src, real_dst)
E           FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp9nfdvti7/download/python' -> '/opt/pipx/py/3.12'

/usr/local/lib/python3.12/shutil.py:886: FileNotFoundError

During handling of the above exception, another exception occurred:

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7f454caa10d0>, mocked_github_api = None

    def test_fetch_missing_python(monkeypatch, mocked_github_api):
        def which(name):
            return None

        monkeypatch.setattr(shutil, "which", which)

        major = sys.version_info.major
        minor = sys.version_info.minor
        target_python = f"{major}.{minor}"

        if target_python == "3.8":
            # 3.8 is not available in the standalone python project
            with pytest.raises(InterpreterResolutionError) as e:
                find_python_interpreter(target_python, fetch_missing_python=True)
                assert "not found" in str(e)
        else:
>           python_path = find_python_interpreter(target_python, fetch_missing_python=True)

tests/test_interpreter.py:191:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
src/pipx/interpreter.py:67: in find_python_interpreter
    standalone_executable = download_python_build_standalone(python_version)
src/pipx/standalone_python.py:90: in download_python_build_standalone
    shutil.move(extracted_dir, install_dir)
/usr/local/lib/python3.12/shutil.py:902: in move
    copytree(src, real_dst, copy_function=copy_function,
/usr/local/lib/python3.12/shutil.py:600: in copytree
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
/usr/local/lib/python3.12/shutil.py:498: in _copytree
    os.makedirs(dst, exist_ok=dirs_exist_ok)
<frozen os>:215: in makedirs
    ???
<frozen os>:215: in makedirs
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

name = '/opt/pipx', mode = 511, exist_ok = False

>   ???
E   PermissionError: [Errno 13] Permission denied: '/opt/pipx'

The same tests passes with a root user.

I'm checking for the core issue but from what it looks like it's not the problem of /opt path existance on MacOS as many global tests are passing apart from this single test.

EDIT: From what it looks like the is_global is set in one of the previous tests and it has the influence on the following test_fetch_missing_python test which shouldn't be affected. Running just test_fetch_missing_python makes it pass, but running all in series causes test_fetch_missing_python to fail :(

Jendker · 2024-03-10T21:01:28Z

On a separate branch just to test what I said above I have added a call in the failing MacOS test to set paths to local (which resets what previous tests did) and now MacOS tests are passing.

Commit: https://github.com/Jendker/pipx/commit/969f0390df81197a99a871c32153a6355e79c65a
Test run: https://github.com/Jendker/pipx/actions/runs/8224676966

It doesn't solve the core issue with pytest, is only a workaround. I don't have enough expertise in pytest to understand why that's needed in the first place but if you think that this test workaround is good enough for now I'll add this commit to PR branch and I think we can merge.

* Added --global switch that sets the relevant paths to point to locations that should be accessable to all users. * Added --global specific tests * The implementation is crude.

...by not having --global in windows The behavior intended by --global doesn't have a precedent in windows when using pip (as in running pip with sudo). Leaving windows support to another time after some discussion.

* Moved path management to `pipx.paths` * Path "constants" available via a context object `pipx.paths.ctx`

Gitznik · 2024-03-13T16:56:27Z

I have tried moving the make_local into conftest but that isn't enough. Failing test: https://github.com/Jendker/pipx/actions/runs/8267713069/job/22618840110 On rebase_main (PR branch) I have added the make_local on fixture cleanup commit while still keeping the make_local at the end of all --global tests and that obviously still works https://github.com/Jendker/pipx/actions/runs/8267933048.

Do you have any suggestions how to improve the situation?

test_environment test_cli_global does not use pipx_temp_env, so that will still leak into one other test. Maybe adding pipx_temp_env there will fix the tests?

This reverts commit fe24c8f.

Jendker · 2024-03-13T17:21:05Z

That's a good point but that's still something the developer needs to add to avoid tests leaking so there is no substantial difference between adding make_local or pipx_temp_env to each test so it doesn't fix the core issue. Please correct me if I'm missing something.

Gitznik · 2024-03-13T17:51:14Z

That's a good point but that's still something the developer needs to add to avoid tests leaking so there is no substantial difference between adding make_local or pipx_temp_env to each test so it doesn't fix the core issue. Please correct me if I'm missing something.

Yes you're right. A lot of test have pipx_temp_env anyways, so having it there just allows you to not worry about make_local - most of the time. But yeah, having it in the tests that make use of the --global flag makes it very obvious that it needs to be done.

I'm fine with either version

Jendker · 2024-03-13T20:19:52Z

I have removed all the explicit make_local calls in the tests and used pipx_temp_env in all tests with --global. I had to update test_cli_global test to reflect new paths from pipx_temp_env. Please let me know if you have any suggestions.

Gitznik

Please add a quick note to the ensurepath docs that you need to run it with --global to add the global location to the PATH. Aside from that - looks good to me know. Great work :)

tests/conftest.py

Jendker · 2024-03-14T21:58:39Z

Please add a quick note to the ensurepath docs that you need to run it with --global to add the global location to the PATH. Aside from that - looks good to me know. Great work :)

Done in 740b775 :)

README.md

docs/installation.md

README.md

mairas · 2024-03-15T17:42:24Z

I'm a bit late with the comment, but /opt is a thing on Macos, at least in practice. Specifically, Homebrew does install itself on /opt/homebrew. You need to create /opt/pipx with sudo and then set the permissions, but other than that, it works fine and I've already installed some packages there with the environment variable hacks described in Issue #754.

Homebrew used to install itself in /usr/local but switched the location some years ago. Don't remember the specifics but I think Macos wasn't happy with relaxing permisssions of /usr/local like Homebrew did. Risking a bit of cargo cult programming here, my preference would be to stick with /opt/pipx.

Jendker · 2024-03-18T05:09:01Z

If all looks good to you can this be merged? I am worried about the possible conflicts with other PR which may emerge if it doesn’t get merged into master some time soon ☺️

Jendker · 2024-03-18T14:40:57Z

Thank you @Gitznik and @chrysle for your help in improving this PR and merging!

Gitznik · 2024-03-18T16:34:54Z

Thanks for picking this up and following all the way through @Jendker 🚀

gaby · 2024-03-19T03:54:31Z

@Gitznik Is there an ETA for the next release? I can't wait to try this feature.

Gitznik · 2024-03-19T07:17:09Z

@Gitznik Is there an ETA for the next release? I can't wait to try this feature.

@gaby There is not concrete ETA, but we plan to have a release very soon :)

chrysle · 2024-03-19T14:20:54Z

And until I've refurbished the release pipeline to do so, please feel free to install directly from the main branch. Pre-release testing is always welcome!

chrysle reviewed Mar 7, 2024

View reviewed changes

Gitznik requested changes Mar 7, 2024

View reviewed changes

tests/test_list.py Outdated Show resolved Hide resolved

tests/test_uninstall.py Outdated Show resolved Hide resolved

Jendker requested review from chrysle and Gitznik March 9, 2024 07:50

chrysle reviewed Mar 9, 2024

View reviewed changes

tests/helpers.py Outdated Show resolved Hide resolved

Jendker requested a review from chrysle March 9, 2024 08:25

Gitznik requested changes Mar 9, 2024

View reviewed changes

docs/installation.md Outdated Show resolved Hide resolved

docs/installation.md Outdated Show resolved Hide resolved

docs/how-pipx-works.md Show resolved Hide resolved

docs/installation.md Outdated Show resolved Hide resolved

Jendker requested a review from Gitznik March 9, 2024 18:03

haxwithaxe and others added 10 commits March 11, 2024 11:15

The bare minimum implementation of --global

45541e9

* Added --global switch that sets the relevant paths to point to locations that should be accessable to all users. * Added --global specific tests * The implementation is crude.

Fixing failing windows tests

bb73423

...by not having --global in windows The behavior intended by --global doesn't have a precedent in windows when using pip (as in running pip with sudo). Leaving windows support to another time after some discussion.

Cleaned up the changes made to implement --global

b3048b5

* Moved path management to `pipx.paths` * Path "constants" available via a context object `pipx.paths.ctx`

Fixed some stuff missed in the merge

627b421

Fix rebase issues

8335f03

Add missing mkdir calls, fix test_upgrade

967f644

Add feature note to changelog.d

d5471a9

Use decorator for test skips on windows

0b7ed40

Global tests update, PR cleanup

366c727

Add note about --global switch in installation docs

453e74c

Revert "Fix ensurepath"

82badc7

This reverts commit fe24c8f.

Remove explicit make_local from tests

3e965b1

Gitznik requested changes Mar 14, 2024

View reviewed changes

chrysle reviewed Mar 14, 2024

View reviewed changes

tests/conftest.py Show resolved Hide resolved

Jendker added 2 commits March 14, 2024 17:02

Inform user about sudo pipx ensurepath --global

740b775

Make test_cli test less tolerant

9597273

chrysle reviewed Mar 15, 2024

View reviewed changes

README.md Show resolved Hide resolved

docs/installation.md Outdated Show resolved Hide resolved

Gitznik reviewed Mar 15, 2024

View reviewed changes

README.md Show resolved Hide resolved

Improve grammar

30a891c

Gitznik approved these changes Mar 16, 2024

View reviewed changes

chrysle approved these changes Mar 16, 2024

View reviewed changes

gaby mentioned this pull request Mar 17, 2024

uv pip install --system cannot create directory astral-sh/uv#2059

Open

Merge branch 'main' into rebase_main

14a90a8

Gitznik enabled auto-merge (squash) March 18, 2024 09:13

Gitznik merged commit 24e8e29 into pypa:main Mar 18, 2024
12 checks passed

adamjorr mentioned this pull request Apr 8, 2024

pipx.util circular import #1333

Open

chrysle mentioned this pull request Apr 21, 2024

Option to install package globally for multi-user access #754

Closed

notatallshaw mentioned this pull request Apr 25, 2024

Best practices doc for venv installs pypa/pip#12651

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated PR: Added --global option to perform actions for all users (#754) #1281

Updated PR: Added --global option to perform actions for all users (#754) #1281

Jendker commented Mar 7, 2024

Jendker commented Mar 7, 2024

chrysle left a comment

Gitznik left a comment

Jendker commented Mar 9, 2024 •

edited

Loading

chrysle commented Mar 9, 2024

Gitznik left a comment

Jendker commented Mar 9, 2024

Gitznik commented Mar 9, 2024 •

edited

Loading

chrysle commented Mar 10, 2024

Gitznik commented Mar 10, 2024

chrysle commented Mar 10, 2024

Gitznik commented Mar 10, 2024

Jendker commented Mar 10, 2024 •

edited

Loading

Jendker commented Mar 10, 2024 •

edited

Loading

Jendker commented Mar 10, 2024

Gitznik commented Mar 13, 2024

Jendker commented Mar 13, 2024

Gitznik commented Mar 13, 2024

Jendker commented Mar 13, 2024 •

edited

Loading

Gitznik left a comment

Jendker commented Mar 14, 2024 •

edited

Loading

mairas commented Mar 15, 2024

Jendker commented Mar 18, 2024

Jendker commented Mar 18, 2024

Gitznik commented Mar 18, 2024

gaby commented Mar 19, 2024

Gitznik commented Mar 19, 2024

chrysle commented Mar 19, 2024

Updated PR: Added --global option to perform actions for all users (#754) #1281

Updated PR: Added --global option to perform actions for all users (#754) #1281

Conversation

Jendker commented Mar 7, 2024

Summary of changes

Test plan

Jendker commented Mar 7, 2024

chrysle left a comment

Choose a reason for hiding this comment

Gitznik left a comment

Choose a reason for hiding this comment

Jendker commented Mar 9, 2024 • edited Loading

chrysle commented Mar 9, 2024

Gitznik left a comment

Choose a reason for hiding this comment

Jendker commented Mar 9, 2024

Gitznik commented Mar 9, 2024 • edited Loading

chrysle commented Mar 10, 2024

Gitznik commented Mar 10, 2024

chrysle commented Mar 10, 2024

Gitznik commented Mar 10, 2024

Jendker commented Mar 10, 2024 • edited Loading

Jendker commented Mar 10, 2024 • edited Loading

Jendker commented Mar 10, 2024

Gitznik commented Mar 13, 2024

Jendker commented Mar 13, 2024

Gitznik commented Mar 13, 2024

Jendker commented Mar 13, 2024 • edited Loading

Gitznik left a comment

Choose a reason for hiding this comment

Jendker commented Mar 14, 2024 • edited Loading

mairas commented Mar 15, 2024

Jendker commented Mar 18, 2024

Jendker commented Mar 18, 2024

Gitznik commented Mar 18, 2024

gaby commented Mar 19, 2024

Gitznik commented Mar 19, 2024

chrysle commented Mar 19, 2024

Jendker commented Mar 9, 2024 •

edited

Loading

Gitznik commented Mar 9, 2024 •

edited

Loading

Jendker commented Mar 10, 2024 •

edited

Loading

Jendker commented Mar 10, 2024 •

edited

Loading

Jendker commented Mar 13, 2024 •

edited

Loading

Jendker commented Mar 14, 2024 •

edited

Loading