-
Notifications
You must be signed in to change notification settings - Fork 759
Mention pypi / pipx as simple installation option #487
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
Conversation
Hi, sorry for the delay. Mentioning pipx in the documentation makes a lot of sense, but not in README.md; it belongs in INSTALL.md. |
I think it makes sense to at least mention in the readme that the package is available via pypi. If you prefer I just keep the info about availability on pypi in README.md and move the hint on pipx to INSTALL.md |
I'd rather both were moved to INSTALL.md. For many, they'll install via apt/yum/dnf/brew/pacman/nix/pypi/pipx/etc. Everyone who uses one of these will believe their preference should get special mention, but I see no reason to call out one above the others. |
I moved the notes to INSTALL.md - hope it´s fine now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking a long time to respond. Took a while to try to put my problems with the patch into words and dig up the historical visceral reaction I have against pip.
INSTALL.md
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
* [Pre-requisites](#pre-requisites) | |||
* [Simple Installation](#simple-installation) | |||
* [Installation as Python Package from PyPI](#installation-as-python-package-from-pypi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should come before the "Installation via Package Manager" section. The historical problems with pip, and pipx's close relationship to pip, are enough to make me not trust pipx much until it proves itself it. As such, it should not be high on the recommendation list to start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The order should reflect Unix/Mac (not Windows). I will wait a bit if you have further comments below and then change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the Unix/Mac comment. Some package managers in the package managers list are Windows-specific. And pipx works on Unix/Mac. So the order of those two things says nothing about operating system. Further, the rationale for putting pipx later was because of its relationship to pip, which specifically made things worse for Windows users (not Mac or Linux users), when we suggested it once upon a time, so putting it later is very specifically in consideration of Windows.
INSTALL.md
Outdated
|
||
An alternative is using [pipx](https://pypa.github.io/pipx/) that is made to | ||
simplify installing and running Python-scripts. It does also work for | ||
`git-filter-repo` on Windows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first sentence is really bad; it's not merely orthogonal to reality, it's 180 degrees out of phase with it. We could rewrite this something like:
Installing `git-filter-repo` via pip often causes all kinds of problems for users:
* https://github.com/newren/git-filter-repo/issues/5#issuecomment-618981046
* https://github.com/newren/git-filter-repo/issues/87#issue-616956065
* https://github.com/newren/git-filter-repo/issues/104#issue-630724338
* https://github.com/newren/git-filter-repo/issues/124#issuecomment-695109714
* https://github.com/newren/git-filter-repo/issues/189#issue-782155667
* https://github.com/newren/git-filter-repo/issues/193#issue-791024768
pip should not be used unless you are comfortable debugging _on your own_; do not file requests for help installing with it. We're extremely worried that pipx will have some of the same problems, but are reluctantly mentioning it as an option since some users have had good luck with it, but if we start getting reports about problems with it we'll rip it out of this install guide too.
That's probably way too overbearing, but there's no way I'm merging anything claiming that pip works well, since it just results in lots of bug reports from users about how it doesn't. I'm also worried that mentioning pipx will cause some users who don't have pipx installed but do have pip to think they can just install with pip, but not understand the differences and run into problems, so I really want any mention of pipx to come with a strong disclaimer against using pip.
Even with both of those I'm a bit worried, because marking files as executable was not the only problem folks had with pip, but it's the only one I think pipx will solve. Granted, that was a big issue, but it's still only one. That said, I'm willing to give it a try, but not if pipx is highly recommended nor if it intentionally or accidentally comes with an endorsement of using pip to install git-filter-repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, of course I was trying to comment on lines 160-166, and the "first line" I referred to was line 160, but GitHub unhelpfully only prints lines 163-166 for context when showing this review on the main page. Ugh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python on Windows is slightly different than on Unix. There is no python3 or pip3 and also the shebang line is mainly a unix feature (although the launcher interprets it). All issues linked above are the result from not knowing the basics of Python on Windows. They are not related to pip or to your project. Exactly he same problems would be observed e.g. when installing poetry. Poetry also suggests installing via pipx.
Regarding the first sentence: The people reporting the issues did not do what the first sentence (line 160) proposes. Nevertheless, after reading the issues I think it is better to propose using pipx
(or the new uv tool) as best pick for Windows and remove or de-emphasize the first sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, you did mention into a virtual environment
. I should read more carefully; sorry about that. (Though that does suggest it was too easy to miss.)
I agree that the problems were related to users "not knowing the basics of Python on Windows", but the problem was I shouldn't be responsible for educating users on those basics; that should be a thing for the Python project. But adding instructions to INSTALL.md suggesting users can install via pip (as I did at request of users in the past), resulted in more complaints about the difficult of installing from Windows users and somehow just confused them worse and they didn't go to Python to learn how to do those basics, they asked all their questions of this single-developer (mostly unfunded) project. At one point I wrote in a commit message:
Be more explicit about how pip is lame and provides virtually no benefit
since it leaves you to fix your $PATH yourself, which was the _only_ step
that was needed in installing the whole package anyway.
before I later simply removed pip from INSTALL.md entirely (which resulted in getting fewer complaints about the difficulty of installing).
Anyway, thanks for listening to me rant. While I'm totally unwilling to entertain pip any further (even in a virtual environment), I'm willing to give pipx a try. I'll look forward to your updated patch.
Your rant made me understand where you come from. I updated the PR to promise less and also removed suggesting a pip-install. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looking better. Other than the two comments on the content changes:
- The final commit is missing your Signed-off-by
- The commit history should be cleaned up -- it shouldn't show the history of how you ended up with your final changes, but should show a history detailing the most logical way from your starting point to your ending point. Thus, instead of doing something in commit C and undoing it in commit F, rebase the series and force push. If you all your changes logically fit in a single patch, then squash them, describe the changes well in the commit message, and force push. This way, when I merge it and others read the history, they can quickly learn how and why we got from point A to point B without intermediate meanderings. (I'd probably suggest 2 commits for this PR -- one to fix the typo, and the other commit for the remainder of the changes, though I could see arguments for 1 or 3 patches as well.)
Hi, almost perfect. Two small things:
Then it should be ready to merge. |
Signed-off-by: David Linke <[email protected]>
Signed-off-by: David Linke <[email protected]>
I rebased, cleaned up the history to just two commits and changed the notes to also mention scoop as you suggested. |
From the issues and
INSTALL.md
one gets the impression that installation on windows is troublesome. However, the package can be simply installed with pipx on Windows. Since pipx is a solid widely used tool mentioning this option could help.