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

Bump userpath to 1.9.0 #1021

Closed
wants to merge 1 commit into from
Closed

Bump userpath to 1.9.0 #1021

wants to merge 1 commit into from

Conversation

dukecat0
Copy link
Member

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

Summary of changes

Bump userpath to 1.9.0

Test plan

Tested by running

# command(s) to exercise these changes

@uranusjr
Copy link
Member

Why do we need to raise the lower bound?

@chrysle
Copy link
Contributor

chrysle commented Jul 24, 2023

I guess because of ofek/userpath#48 and #996.

@safl
Copy link

safl commented Jul 26, 2023

Hmm, have you considered that userpath v1.9.0 breaks automation use-cases?
Specifically I have this issue: ofek/userpath#50 with it.

I would propose:

userpath>=1.6.0<=1.8.0

This is an issue as-is, when installing pipx via pip as it grabs the latest userpath module, and thus renders cli-tools installed via pipx and PATH-updated with pipx ensurepath unusable.

@uranusjr
Copy link
Member

I don’t feel adding an upper cap would be a good idea since it would require a new release when say 1.10 come out. You can add that upper limit yourself if it suits your use case, but someone else can’t remove one if that doesn’t work for them.

@safl
Copy link

safl commented Jul 27, 2023

I don’t feel adding an upper cap would be a good idea since it would require a new release when say 1.10 come out. You can add that upper limit yourself if it suits your use case, but someone else can’t remove one if that doesn’t work for them.

  • An upper-bound would prevent a functional regression like the one that is happening right now

    • A new release of pipx would not be required when userpath is released, unless it provides functionality needed by pipx. What could have happened here is that someone might have said "oh wait, bumping userpath to v1.9.0 introduces a regression in the behavior of pipx ensurepath, let's resolve that before bumping". Or, if there were tests for non-login shells, then they would catch it. Rather than the functional regression seeping into pipx.
    • I guess it is a trade-off between "conveniently" getting third-party code bumped automatically with the risk of pipx sporadically breaking / altering behavior (for the same version of pipx) vs the chore of verifying changes to third-party code with the upside of stable / non-altering behavior (for the same version of pipx)
  • I cannot control the version of dependencies when installing pipx via the system package manager

    • With the introduction of https://peps.python.org/pep-0668/ then pipx is a convenient way to manage command-line tools implemented in Python. However, with the regression introduced by userpath v1.9.0 then it no longer applies for non-login shells, so basically if you run your commands via SSH-exec (ssh user@host "my_command").
    • Installing pipx via the system package manager is a really convenient way to bootstrap the availability of Python-based command-line tools and with pipx ensurepath even more so
    • Thus, for example on Debian, when https://packages.debian.org/bookworm/python3-userpath is upgraded to v1.9.0, then using pipx ensurepath won't be usable for non-login shells any longer, I won't be able to fix that.
    • As I understand the motivation behind userpath dropping non-login shells, then it was not because something was "not working" but rather that it was "not nice that there where duplicate entries in the PATH variable. I might be wrong, but I just weigh functional regressions higher than nice-to-haves.

I have opened up issue #1025 to better describe the use-case.

@dukecat0
Copy link
Member Author

dukecat0 commented Aug 5, 2023

Why do we need to raise the lower bound?

Originally I thought this would fix #978 and #1007, but seems like now it can lead to more issues, so I am closing this PR.

@dukecat0 dukecat0 closed this Aug 5, 2023
@dukecat0 dukecat0 deleted the bump-userpath branch August 5, 2023 11:47
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.

None yet

4 participants