Use setup.py entry_points for installation#217
Conversation
|
Sweet! Thanks for tracking that down and providing a fix! Could you make two small changes? One, add a Signed-off-by line to your commit message as per https://git.kernel.org/pub/scm/git/git.git/tree/Documentation/SubmittingPatches#n303. Two, can you modify the commit message to avoid mentioning "#144" and instead just (briefly) describe the problem being fixed instead? I'd rather the commit messages told the story without the need for users to look up issue links, especially if filter-repo ever moves hosting providers. (If you still want to mention "#144", feel free to change the description of this pull request to mention it.). Thanks again! |
Sure, I'll do that. However, the pipeline tests seem to be unhappy with my changes. But by looking at the failed tests I'm not sure what's the issue there - I can't connect the errors to my changes. Do I oversee something? |
I don't see the errors locally either, so I suspect something is up with the GitHub Actions runners. Just ignore it for now. |
newren
left a comment
newren
left a comment
There was a problem hiding this comment.
The first patch looks good and I'd love to include it, but I still need you to add your Signed-off-by to the commit. If you do that one step, I'm happy to fix up anything and everything else and merge your changes.
If you want to go above and beyond, other things to tweak:
- Drop the two last patches (there's no point adding a patch and then immediately reverting it)
- Change the commit message to not mention "#144" but instead just describe the problem and the fix so it is self contained.
- Double-check that the pipeline of tests fail (I suspect they were just flaking in the past when you ran them)
but I'm happy to fix all three of these up for you if you just modify the commit to include your Signed-off-by.
|
I figured out the test failures, and it was just a pre-existing bug in git-filter-repo where I didn't explicitly .close() some files but Python just happened to finish writing them all out anyway. For whatever reason, your tiny tweak made it so Python stopped doing that, but I should have been closing it all along. Fixed in commit 7ceb213 (filter-repo: ensure we close files so they get written, 2021-06-08). Rebasing your work on top of that commit makes the tests all pass. As I said above, if you add your Signed-off-by to the commit message, I'll be happy to clean up anything else and merge the change. |
3a8366b to
5c6f41a
Compare
|
After I had another look at the installation procedure I wasn't entirely sure that I did things nice and clean in my patch; that's why I was hesitant to go forward. Anyway, I added the sign-off now, so if you feel confident about my patch feel free to merge it. |
This should make the installation via pip more robust.
On Windows the usage of entry_points will install a wrapper executable
for the script that chooses the proper python executable. This
essentially makes the script run correctly when called via `git
filter-repo` (direct execution via `git-filter-repo` was already fine
before).
This fixes an issue on Windows, where the git-installation will choose a
different python executable than the one indicated by the installation
via `pip{x,3} install`.
Signed-off-by: Benjamin Motz <[email protected]>
5c6f41a to
4ff15cd
Compare
|
Thanks for the contribution! |
2 participants
This should make the installation via pip more robust.
On Windows the usage of entry_points will install a wrapper executable
for the script that chooses the proper python executable. This
essentially makes the script run correctly when called via
git filter-repo(direct execution viagit-filter-repowas already finebefore).
This fixes #144 where git (on Windows) chooses a different python
executable than the one indicated by the installation via
pip{x,3} install.I have no overview of all the installation methods and how this PR might
influence them. Also, I obviously couldn't verify the entire deployment process
to PyPI, so this still needs to be tested. I could only check that the created
wheel installs properly on Windows (with some local modifications to the
Makefile, as it seems to be designed for usage on Linux only).