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

Fallback to user's log path if default log path is not writable #870

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Fallback to user's log path if default log path is not writable #870

merged 4 commits into from
Mar 13, 2023

Conversation

Jackenmen
Copy link
Contributor

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

Summary of changes

This PR changes the logging setup so that it fallbacks to user's log path (as defined by platformdirs.user_log_path("pipx")) if the default log path ($PIPX_HOME/logs) is not writable which may occur when using pipx for install applications system-wide (per the documentation here: https://pypa.github.io/pipx/installation/#installation-options). This allows the user to use commands such as pipx list to list system-wide applications without having access to $PIPX_HOME.
This was suggested in #754 (comment)

While working on this, I had a few thoughts/questions:

  • Should constants.PIPX_LOG_DIR be altered or actually remain a constant as the module name advertises (note that constants.pipx_log_dir is going to be altered anyway)
    • currently, I went with not touching it, this is reflected in the test plan below
  • Are we sure that we want to use platformdirs here:
    • it wasn't already a dependency
    • a fallback that also makes some amount of sense would be using $DEFAULT_PIPX_HOME/logs instead
  • Should pipx perhaps capture all OSErrors at the top level? Seems kind of like scope creep for this PR but in general, this is something that pip does and seems to make sense

There's a single test failure on jaraco-financial that is unrelated to this PR:

`jaraco-financial` failure
pipx >(setup:810): pipx version is 1.1.0.1.dev0
pipx >(setup:811): Default python interpreter is '/Users/runner/work/pipx/pipx/.nox/tests-3-10/bin/python'
pipx >(mkdir:81): creating directory /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/subdir/pipxhome/venvs
pipx >(mkdir:81): creating directory /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/otherdir/pipxbindir
pipx >(mkdir:81): creating directory /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/subdir/pipxhome/.cache
pipx >(package_name_from_spec:323): Determined package name: jaraco-financial
pipx >(package_name_from_spec:324): Package name determined in 0.0s
creating virtual environment...
pipx >(run_subprocess:173): running /Users/runner/work/pipx/pipx/.nox/tests-3-10/bin/python -m venv --without-pip /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/subdir/pipxhome/venvs/jaraco-financial
pipx >(run_subprocess:173): running /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/subdir/pipxhome/venvs/jaraco-financial/bin/python -c import sysconfig; print(sysconfig.get_path('purelib'))
pipx >(run_subprocess:173): running /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/session_shareddir0/bin/python -c import sysconfig; print(sysconfig.get_path('purelib'))
pipx >(run_subprocess:173): running /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/subdir/pipxhome/venvs/jaraco-financial/bin/python --version
pipx >(_parsed_package_to_package_or_url:128): cleaned package spec: jaraco-financial==2.0.0
installing jaraco-financial from spec 'jaraco-financial==2.0.0'...
pipx >(run_subprocess:173): running /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/subdir/pipxhome/venvs/jaraco-financial/bin/python -m pip install jaraco-financial==2.0.0
pipx >(subprocess_post_check_handle_pip_error:335): '/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/subdir/pipxhome/venvs/jaraco-financial/bin/python -m pip install jaraco-financial==2.0.0' failed
pipx >(subprocess_post_check_handle_pip_error:352): Fatal error from pip prevented installation. Full pip output in file:
    /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/subdir/pipxhome/logs/cmd_2022-07-13_19.38.45_pip_errors.log
    
    pipx >(analyze_pip_output:310): pip seemed to fail to build package:
    lxml
    
    Some possibly relevant errors from pip install:
    error: subprocess-exited-with-error
    Error: Please make sure the libxml2 and libxslt development packages are installed.
    error: metadata-generation-failed
    pipx >(rmdir:55): removing directory /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/test_name_tricky_characters_ja0/subdir/pipxhome/venvs/jaraco-financial
Error installing jaraco-financial from spec 'jaraco-financial==2.0.0'.

Test plan

Tested by running

# in project's directory
python -m venv .venv
. .venv/bin/activate
python -m pip install .

# using an actual path to the executable as sudo won't use user's `PATH` for security reasons
sudo PIPX_HOME=/opt/pipx PIPX_BIN_DIR=/usr/local/bin .venv/bin/python -m pipx install cowsay

# this should succeed even though the user doesn't have permissions to the default log directory
python -m pipx list

# this outputs `/opt/pipx/logs`
python -m pipx environment --value PIPX_LOG_DIR

@Jackenmen
Copy link
Contributor Author

A fix for the jaraco-financial issue is available at #871.

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Not sure if it’s worthwhile to pull in plarformdirs just for this, but logically lgtm.

@Jackenmen
Copy link
Contributor Author

Not sure if it’s worthwhile to pull in plarformdirs just for this

I wasn't either which is why I mentioned it in the PR description. The alternative that is simple to implement is using $DEFAULT_PIPX_HOME/logs as the fallback since it already is in a directory that should be writable by the current user. Other alternatives would probably end up adding enough complexity to not make it worth maintaining over just using platformdirs directly.

@uranusjr
Copy link
Member

Or we could just copy the code from platformdirs; for log directories specifically the logic is actually pretty straightforward.

@Jackenmen
Copy link
Contributor Author

I merged main to fix the failing check.

Or we could just copy the code from platformdirs; for log directories specifically the logic is actually pretty straightforward.

On Linux and macOS it is, on Windows doing it properly (as platformdirs does) isn't though.

There's also Android where it isn't that simple either but I would guess that you probably don't care about Android 😄

@uranusjr uranusjr merged commit 726eaf8 into pypa:main Mar 13, 2023
@Jackenmen Jackenmen deleted the fallback_to_user_writable_log_directory branch March 13, 2023 08:13
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.

2 participants