* [ITP] git-filter-repo 2.34.0 @ 2022-03-08 0:11 James Morris 2022-03-08 9:28 ` Adam Dinwoodie 2022-03-10 20:52 ` Marco Atzeri 0 siblings, 2 replies; 8+ messages in thread From: James Morris @ 2022-03-08 0:11 UTC (permalink / raw) To: cygwin-apps Hello, I'd like to maintain the package for git-filter-repo, a Python script to quickly edit git history. It's MIT licensed, available in both Debian and Fedora, and I've also just recently packaged git-filter-repo up for MSYS2. Cygport package files can be found here: https://drive.google.com/drive/folders/12RXG0804TmR9ZF2STpV7LXqpJMucYAfx?usp=sharing Thanks, James ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [ITP] git-filter-repo 2.34.0 2022-03-08 0:11 [ITP] git-filter-repo 2.34.0 James Morris @ 2022-03-08 9:28 ` Adam Dinwoodie 2022-03-08 20:42 ` James Morris 2022-03-10 20:52 ` Marco Atzeri 1 sibling, 1 reply; 8+ messages in thread From: Adam Dinwoodie @ 2022-03-08 9:28 UTC (permalink / raw) To: cygwin-apps On Mon, Mar 07, 2022 at 07:11:24PM -0500, James Morris wrote: > Hello, > > I'd like to maintain the package for git-filter-repo, a Python script > to quickly edit git history. It's MIT licensed, available in both > Debian and Fedora, and I've also just recently packaged > git-filter-repo up for MSYS2. Oh excellent! I've been thinking about doing this myself, but I'm very happy for someone else to do the work! > Cygport package files can be found here: > https://drive.google.com/drive/folders/12RXG0804TmR9ZF2STpV7LXqpJMucYAfx?usp=sharing A few thoughts from me before I think this is good to go: - You've patched the shebang from `/usr/bin/env python3` to `/usr/bin/python3`. To what end? /usr/bin/env is part of coreutils for Cygwin, so there shouldn't be any risk that it won't be installed. If someone has their own compiled python3 in /usr/local/bin, they'd probably expect that to be used, so I don't think I'd change the shebang unless there's some clear and specific reason for doing so. - You're changing the shebang with both a patch file and with a line in src_compile; you don't need to do both! I suspect this is an artefact of how Cygport packages the source files, but AIUI the canonical way to do this sort of patching with Cygport is to drop the sed line from your .cygport file and just keep the patch file that gets generated. - You've set the category as both Devel and Python. IMO (I've not checked what the general consensus on this is) this shouldn't be in Python: it's a tool that happens to use Python, but I'd expect the Python category to be for things that are specifically useful to people doing Python dev, so things like libraries that can be usefully imported in a Python module, or tools for debugging Python scripts. I think this should only be in the Devel category. - That said, I think ideally you'd also be packaging git_filter_repo.py, which does provide a Python library that users can import. At that point, this would unambiguously belong in both categories. - You're installing the main script into /usr/bin. I think the executable should probably be installed into /usr/libexec/git-core, along with other Git executables. This is what `git --exec-path` returns, and matches what's described in INSTALL.md. More generally, I think you should be emulating one of the installation mechanisms in INSTALL.md, either using the provided makefile, or following the described steps in the "Manual Installation" section of that doc. - Currently Cygport can't run the test suite. Ideally it'd be able to; it seems unlikely that there would be Cygwin-specific regressions in this code, but it's not out of the question, and given upstream provide a test suite, it seems a shame to not use it. That all said, this is clearly very close to ready to ship, and as I say, it'll definitely be useful once it's available. Thank you! Adam ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [ITP] git-filter-repo 2.34.0 2022-03-08 9:28 ` Adam Dinwoodie @ 2022-03-08 20:42 ` James Morris 2022-03-11 21:54 ` Adam Dinwoodie 0 siblings, 1 reply; 8+ messages in thread From: James Morris @ 2022-03-08 20:42 UTC (permalink / raw) To: cygwin-apps Hi Adam, Thanks for the feedback! > - You've patched the shebang from `/usr/bin/env python3` to > `/usr/bin/python3`. To what end? /usr/bin/env is part of coreutils > for Cygwin, so there shouldn't be any risk that it won't be installed. > If someone has their own compiled python3 in /usr/local/bin, they'd > probably expect that to be used, so I don't think I'd change the > shebang unless there's some clear and specific reason for doing so. I am trying to prevent exactly what you described. git-filter-repo needs Python >=3.5 to function and we know that `/usr/bin/python3` is the correct version. Suppose a user installed Python 3.3 at `/usr/local/bin/python3`, now git-filter-repo will run with the wrong Python version and most likely break. This is what other distributions do when they distribute Python scripts and I'm fairly sure Debian explicitly calls this out in its policy. > - You're changing the shebang with both a patch file and with a line in > src_compile; you don't need to do both! I suspect this is an artefact > of how Cygport packages the source files, but AIUI the canonical way > to do this sort of patching with Cygport is to drop the sed line from > your .cygport file and just keep the patch file that gets generated. Yeah the patch file was automatically generated when I ran `cygport all` and I wasn't sure what to do with it. To me it seems silly to have a patch just to change the shebang line when `sed` works fine. I'll try removing `sed` to see what happens. > - You've set the category as both Devel and Python. IMO (I've not > checked what the general consensus on this is) this shouldn't be in > Python: it's a tool that happens to use Python, but I'd expect the > Python category to be for things that are specifically useful to > people doing Python dev, so things like libraries that can be usefully > imported in a Python module, or tools for debugging Python scripts. I > think this should only be in the Devel category. Yeah I initially didn't have it in the Python category, but then I thought about how other tools like bzr and mercurial are there so it seemed appropriate. Granted I didn't check if they also provided Python libraries, but I thought it made sense to put git-filter-repo in the Python category to maybe warn users that installing it would pull in Python. > - That said, I think ideally you'd also be packaging git_filter_repo.py, > which does provide a Python library that users can import. At that > point, this would unambiguously belong in both categories. I wasn't sure how to go about this since I didn't know if that would mean making a bunch of 'python3*-git-filter-repo' packages. Do you think I should make it importable, remove it from the Python category, or just leave it as is? > - You're installing the main script into /usr/bin. I think the > executable should probably be installed into /usr/libexec/git-core, > along with other Git executables. This is what `git --exec-path` > returns, and matches what's described in INSTALL.md. More generally, > I think you should be emulating one of the installation mechanisms in > INSTALL.md, either using the provided makefile, or following the > described steps in the "Manual Installation" section of that doc. Since git-filter-repo is a third-party command like git-lfs, I am of the opinion that it should go in `/usr/bin` rather than git's private `libexec`. That's also how Debian, Gentoo, Arch, and Homebrew package it. Also I believe my cygport file does pretty much follow the manual installation instructions except for the part about installing git_filter_repo.py (for the reasons I stated above). I would like to use the Makefile, however it has a number of things that make it difficult to work with in this context and from what I've seen basically none of the other packagers use it and instead opt to do the installation manually like I have. > - Currently Cygport can't run the test suite. Ideally it'd be able to; > it seems unlikely that there would be Cygwin-specific regressions in > this code, but it's not out of the question, and given upstream > provide a test suite, it seems a shame to not use it. I'll add that in. The package does currently report some errors with git 2.35 and it has been reported upstream (not by me) https://github.com/newren/git-filter-repo/issues/344. I look forward to getting this in Cygwin! -James ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [ITP] git-filter-repo 2.34.0 2022-03-08 20:42 ` James Morris @ 2022-03-11 21:54 ` Adam Dinwoodie 2022-03-12 12:59 ` Jon Turney 0 siblings, 1 reply; 8+ messages in thread From: Adam Dinwoodie @ 2022-03-11 21:54 UTC (permalink / raw) To: cygwin-apps On Tue, Mar 08, 2022 at 03:42:13PM -0500, James Morris wrote: > Hi Adam, > > Thanks for the feedback! > > > - You've patched the shebang from `/usr/bin/env python3` to > > `/usr/bin/python3`. To what end? /usr/bin/env is part of coreutils > > for Cygwin, so there shouldn't be any risk that it won't be installed. > > If someone has their own compiled python3 in /usr/local/bin, they'd > > probably expect that to be used, so I don't think I'd change the > > shebang unless there's some clear and specific reason for doing so. > > I am trying to prevent exactly what you described. git-filter-repo > needs Python >=3.5 to function and we know that `/usr/bin/python3` is > the correct version. Suppose a user installed Python 3.3 at > `/usr/local/bin/python3`, now git-filter-repo will run with the wrong > Python version and most likely break. This is what other distributions > do when they distribute Python scripts and I'm fairly sure Debian > explicitly calls this out in its policy. I just went and did an entirely unscientific check of the scripts I have installed in my Cygwin /bin/ directory. It looks like there's no great consistency, but the majority of scripts there (20 to 9) are calling the relevant Python interpreter directly rather than relying on env. Personally, I'd probably not bother changing things from the upstream package, but if you'd rather do things this way I'm not going to argue :) > > - You're changing the shebang with both a patch file and with a line in > > src_compile; you don't need to do both! I suspect this is an artefact > > of how Cygport packages the source files, but AIUI the canonical way > > to do this sort of patching with Cygport is to drop the sed line from > > your .cygport file and just keep the patch file that gets generated. > > Yeah the patch file was automatically generated when I ran `cygport > all` and I wasn't sure what to do with it. To me it seems silly to > have a patch just to change the shebang line when `sed` works fine. > I'll try removing `sed` to see what happens. Cygport automatically generates patches when it detects a difference between the "src" and "origsrc" directories. You're changing something in "src", so the patch gets generated. The idea is that you can adjust things in the src directory by hand, then when you run cygport it'll automatically store the diffs so you never need to make the same changes again. I suspect the best solution here would be to either (a) drop the sed line and just use a patch file, or (b) make the change in the inst directory as part of the `src_install` function in the .cygport file, i.e. fixing it up as part of doing the "installation" step rather than the "compilation" step. But the sed command is idempotent, so while having both is redundant and a bit odd, I don't think it does any harm. > > - You've set the category as both Devel and Python. IMO (I've not > > checked what the general consensus on this is) this shouldn't be in > > Python: it's a tool that happens to use Python, but I'd expect the > > Python category to be for things that are specifically useful to > > people doing Python dev, so things like libraries that can be usefully > > imported in a Python module, or tools for debugging Python scripts. I > > think this should only be in the Devel category. > > Yeah I initially didn't have it in the Python category, but then I > thought about how other tools like bzr and mercurial are there so it > seemed appropriate. Granted I didn't check if they also provided > Python libraries, but I thought it made sense to put git-filter-repo > in the Python category to maybe warn users that installing it would > pull in Python. I've just checked both bzr and mercurial, and they definitely do provide Python packages. I don't think it's necessary to warn users about dependencies by using categories; setup provides those warnings already when it does dependency resolution. > > - That said, I think ideally you'd also be packaging git_filter_repo.py, > > which does provide a Python library that users can import. At that > > point, this would unambiguously belong in both categories. > > I wasn't sure how to go about this since I didn't know if that would > mean making a bunch of 'python3*-git-filter-repo' packages. > Do you think I should make it importable, remove it from the Python > category, or just leave it as is? My preference here would be to make it importable. That's not going to be something many people are interested in, but there's no reason not to. It can still be a single package -- as bzr and mercurial are -- providing both the main executable and the Python libraries. There's obviously a balance here: monolith packages that add a bunch of dependencies or eat a bunch of disk space / bandwidth, to provide functions many users won't care about, are clearly a bad idea. But the cost of having both the Python module and the executable in this package is going to be a few hundred bytes and no extra dependencies, so I think just providing the lot in one bundle is the way to go. > > - You're installing the main script into /usr/bin. I think the > > executable should probably be installed into /usr/libexec/git-core, > > along with other Git executables. This is what `git --exec-path` > > returns, and matches what's described in INSTALL.md. More generally, > > I think you should be emulating one of the installation mechanisms in > > INSTALL.md, either using the provided makefile, or following the > > described steps in the "Manual Installation" section of that doc. > > Since git-filter-repo is a third-party command like git-lfs, I am of > the opinion that it should go in `/usr/bin` rather than git's private > `libexec`. That's also how Debian, Gentoo, Arch, and Homebrew package > it. Also I believe my cygport file does pretty much follow the manual > installation instructions except for the part about installing > git_filter_repo.py (for the reasons I stated above). I would like to > use the Makefile, however it has a number of things that make it > difficult to work with in this context and from what I've seen > basically none of the other packagers use it and instead opt to do the > installation manually like I have. You're quite right. I'd expected most other packagers to put it in the git-core path, but that's clearly not the norm, and I think it makes sense to follow the lead of other distros here. > > - Currently Cygport can't run the test suite. Ideally it'd be able to; > > it seems unlikely that there would be Cygwin-specific regressions in > > this code, but it's not out of the question, and given upstream > > provide a test suite, it seems a shame to not use it. > > I'll add that in. The package does currently report some errors with > git 2.35 and it has been reported upstream (not by me) > https://github.com/newren/git-filter-repo/issues/344. Magic, thank you! Adam ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [ITP] git-filter-repo 2.34.0 2022-03-11 21:54 ` Adam Dinwoodie @ 2022-03-12 12:59 ` Jon Turney 0 siblings, 0 replies; 8+ messages in thread From: Jon Turney @ 2022-03-12 12:59 UTC (permalink / raw) To: cygwin-apps On 11/03/2022 21:54, Adam Dinwoodie wrote: > >>> - You're changing the shebang with both a patch file and with a line in >>> src_compile; you don't need to do both! I suspect this is an artefact >>> of how Cygport packages the source files, but AIUI the canonical way >>> to do this sort of patching with Cygport is to drop the sed line from >>> your .cygport file and just keep the patch file that gets generated. >> >> Yeah the patch file was automatically generated when I ran `cygport >> all` and I wasn't sure what to do with it. To me it seems silly to >> have a patch just to change the shebang line when `sed` works fine. >> I'll try removing `sed` to see what happens. > > Cygport automatically generates patches when it detects a difference > between the "src" and "origsrc" directories. You're changing something > in "src", so the patch gets generated. The idea is that you can adjust > things in the src directory by hand, then when you run cygport it'll > automatically store the diffs so you never need to make the same changes > again. You can use DIFF_EXCLUDES to ignore that file for patch generation, if it's being modified in the src_compile(), like this. (which seems to be now present in the cygport when I looked it over) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [ITP] git-filter-repo 2.34.0 2022-03-08 0:11 [ITP] git-filter-repo 2.34.0 James Morris 2022-03-08 9:28 ` Adam Dinwoodie @ 2022-03-10 20:52 ` Marco Atzeri 2022-03-11 13:37 ` Jon Turney 1 sibling, 1 reply; 8+ messages in thread From: Marco Atzeri @ 2022-03-10 20:52 UTC (permalink / raw) To: cygwin-apps On 08.03.2022 01:11, James Morris wrote: > Hello, > > I'd like to maintain the package for git-filter-repo, a Python script > to quickly edit git history. It's MIT licensed, available in both > Debian and Fedora, and I've also just recently packaged > git-filter-repo up for MSYS2. > > Cygport package files can be found here: > https://drive.google.com/drive/folders/12RXG0804TmR9ZF2STpV7LXqpJMucYAfx?usp=sharing > > Thanks, > James added to the package list. As soon you and Adam are aligned, you can upload. https://cygwin.com/packaging-contributors-guide.html ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [ITP] git-filter-repo 2.34.0 2022-03-10 20:52 ` Marco Atzeri @ 2022-03-11 13:37 ` Jon Turney 2022-04-08 10:26 ` Jon Turney 0 siblings, 1 reply; 8+ messages in thread From: Jon Turney @ 2022-03-11 13:37 UTC (permalink / raw) To: James Morris, cygwin-apps On 10/03/2022 20:52, Marco Atzeri wrote: > On 08.03.2022 01:11, James Morris wrote: >> Hello, >> >> I'd like to maintain the package for git-filter-repo, a Python script >> to quickly edit git history. It's MIT licensed, available in both >> Debian and Fedora, and I've also just recently packaged >> git-filter-repo up for MSYS2. >> >> Cygport package files can be found here: >> https://drive.google.com/drive/folders/12RXG0804TmR9ZF2STpV7LXqpJMucYAfx?usp=sharing >> >> >> Thanks, >> James > > added to the package list. > As soon you and Adam are aligned, you can upload. > > https://cygwin.com/packaging-contributors-guide.html Please also provide a SSH key as explained here: https://cygwin.com/packaging/key.html ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [ITP] git-filter-repo 2.34.0 2022-03-11 13:37 ` Jon Turney @ 2022-04-08 10:26 ` Jon Turney 0 siblings, 0 replies; 8+ messages in thread From: Jon Turney @ 2022-04-08 10:26 UTC (permalink / raw) To: cygwin-apps, James Morris On 11/03/2022 13:37, Jon Turney wrote: > On 10/03/2022 20:52, Marco Atzeri wrote: >> On 08.03.2022 01:11, James Morris wrote: >>> Hello, >>> >>> I'd like to maintain the package for git-filter-repo, a Python script >>> to quickly edit git history. It's MIT licensed, available in both >>> Debian and Fedora, and I've also just recently packaged >>> git-filter-repo up for MSYS2. >>> >>> Cygport package files can be found here: >>> https://drive.google.com/drive/folders/12RXG0804TmR9ZF2STpV7LXqpJMucYAfx?usp=sharing >>> >>> >>> Thanks, >>> James >> >> added to the package list. >> As soon you and Adam are aligned, you can upload. >> >> https://cygwin.com/packaging-contributors-guide.html > > Please also provide a SSH key as explained here: > > https://cygwin.com/packaging/key.html Ping? ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-08 10:27 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-08 0:11 [ITP] git-filter-repo 2.34.0 James Morris 2022-03-08 9:28 ` Adam Dinwoodie 2022-03-08 20:42 ` James Morris 2022-03-11 21:54 ` Adam Dinwoodie 2022-03-12 12:59 ` Jon Turney 2022-03-10 20:52 ` Marco Atzeri 2022-03-11 13:37 ` Jon Turney 2022-04-08 10:26 ` Jon Turney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).