On Fri, Nov 10, 2023 at 12:05:20AM +0800, Maytham Alsudany wrote: > On Thu, 2023-11-09 at 00:32 +0530, Nilesh Patra wrote: > > > On the contrary, avoiding the use of dh-golang as done in this repo[3] causes > > > all the tests to pass without problem, and I'm unsure to why that is. > > > > This was due to some caveats with the build system and also how > > dh_golang works. We added in a patch that'd skip running gen-go-code.py > > and this was being used at more than one place. > > Doesn't the update_go_generated_files function in setup.py[6] run gen-go- > code.py, specifically `kitty +launch gen-go-code.py`? Patch#0003[7] adds the > `return` statement right afterwards. Yeah, that was a mis-type from my end. I meant to say "build_static_kittens" function instead from where we are returning and building the kitten binary on our own. The line: `"HOME="$(HOME)" KITTY_RUNTIME_DIRECTORY="$(KITTY_RUNTIME_DIRECTORY)" python3 setup.py $(V) build-launcheri` in the tests in d/rules calls "build_static_kittens(args, launcher_dir=launcher_dir)" and this seems to build the kittens binary 'again' in kitty/launcher which did not work for us because we patched out the function altogether. What I did to get around this is to simply symlink the already created kittens binary - that got the python tests passing. Let me know if this is still unclear. > > > We may have to take the approach Fedora has taken, where they've skipped any > > > continuously failing tests[4]. > > > > For now I disabled two tests in the go code that tries to fiddle with > > proc/devfs and can potentially fail in a chroot. Python tests probably > > also try to do some non-standard stuff and we could disable it later if > > it goes flaky on the buildd machines. > > If we have time, it's probably good to get to the bottom of these flaky tests > and patch them instead of skipping them outright. I did not dig very deep but I have some pointers. The two go tests skipped are: * TestCreateAnonymousTempfile - my *hunch* this very likely fails due to it trying to do $things with procfs. This test does not fail for me in my base system but rather in a chroot. * TestHintMarking - I am not sure why this fails but I observed the failure even when I tried in the upstream repo. Not 100% sure if I ran it the right way but since it failed anyway, I considered skipping it. I hope that provides some justification. > This may involve filing bug reports upstream, and > creating PRs when necessary upstream in order to improve > their test suites for both Python and Golang. Agreed, and I would love some coordination/help with this bit. > > That said, I want to discuss/ask a few things: > > * Can you take a look at my commits and let me know if you have any > > comments? > > Regarding commit 38ea7407: > This is a nitpick (and I think this is a mistake): should the patch file end > with .patch instead of .py? This was an oversight at my end - fixed - Thanks. > It would probably be beneficial if we could setup the gbp workflow by hand, > ensuring the upstream branch is up to date Not sure what you mean here. `gbp import-orig` takes care of it. > and setting up a patch-queue/debian/(unstable|experimental) branch. No, please. These are never really meant to be pushed. Besides people use different way to apply patches - I use quilt and not `gbp pq`. > Regarding commit d847dbfe: > The d/ch entry needs some cleaning up, with the merging and removal of some > points. (I'm assuming you used `gbp dch`.) > > For example, > > * patch: make setup.py compatible with GOPATH mode > is probably not something the end user needs to know about. But we (as maintainers) do. I also find changelog a sensible way to keep track of our changes and I find it helpful to go back in time to see when a certain change was done and what was the state of the repository then to compare with the current version so as to re-assess whether the change done back then makes sense now. > Let me know if I should leave it or clean it up. I prefer that you leave it. > Regarding commit 7020dd5d: > Are the ldflags necessary, since the binary builds fine without them set? > Especially the kitty.VCSRevision option, which is only really used in --version, > as shown in this upstream commit[9]. I do not find any commit at [9] but rather the lintian output. I simply decided to emulate the way in which upstream buildsystem would behave to avoid surprises so I'd be OK with keeping it that way. LMK if you disagree. > > * Can you please clean up some of the lintian stuff? And then we could > > upload the new release. > > The latest lintian report from the (newly discovered) GitLab Salsa pipeline can > be found here[9]. > > I can go through and fix most of the issues lintian reports, but I would like > discuss with you the unusual-interpreter warning in the kitty binary package. > All the kitty/**/*.py files are using a `#!/usr/bin/env python` shebang, whereas > lintian wants `#!/usr/bin/env python3`. I'd rather say a `#!/usr/bin/python3` instead. > Should we add a simple recursive find > and replace to debian/rules? WDYT? The problem with such a call is that the package can't build again after the first build due to non-patch changes in the files. So if we are doing a recursive replace, it should be done in the "override_dh_python3" step on the files *inside* debian dir. OTOH, it may be possible to do so "dh_python3 --shebang=/usr/bin/python3" magic to get this fixed. To be clear, recursive find and replace is OK for me as long as it is done in a later step inside the files in debian/ dir. Feel free to go ahead with a fix. That said I do see a directive to "/usr/bin/env python3" in specific files for instance in "kittens/tui/handler.py" "kitty/fonts/render.py" but not in others. IMHO we should ask upstream to maintain uniformity by fixing this. > > * Since we both are interested in kitty's packaging, I think we have two > > options: > > - Either you or me would be in the "Maintainer" field and the other one would > > be in "Uploader" field. > > - Add kitty@packages.debian.org as the maintainer and add both of us > > as uploaders (that means we subscribe to the package email > > ofcourse). > > Which one do you think we should do? > > I don't really have a preference, so I'm happy with whatever you would like to > do regarding that. So I suppose the question now changes to: "Would you like to be responsible to maintain this package and commit to its maintenance as a primary PoC"? > Keep in mind that the contact in the Maintainer field will > pretty much be the "face" of the package. I've maintained a fair bit of python and go packages for a few years even before I was a Debian Developer so I'd be kind of OK with being the maintainer - ofcourse only if you're fine with it. > > * I suppose the maintenance of this package will keep getting messy due > > to upstream mixing up two language build systems in a fairly non-standard way. > > I suppose it makes sense to ask upstream if they'd consider to switch > > to something that eases maintenance burden on us (Debian). WDYT? > > I think that's a good idea, but if we do ask upstream, we should propose a > solution to them to make it as easy as possible for them to implement, and > perhaps some work on our side. > > Perhaps splitting up the kitty and kittens components into separate > folders/repos? Or make it so that these components can build independently of > each other (setup.py builds kitty only, `go build` builds kittens only)? Yes, I agree. While I was doing the work of making the go part work, I felt that it'd indeed be more sensible if "kitten" was a separate repo altogether. > > > [1]: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1037440#37 > > > [2]: https://salsa.debian.org/Maytha8/kitty-dh-golang > > > [3]: https://salsa.debian.org/Maytha8/kitty > > > [4]: https://src.fedoraproject.org/rpms/kitty/blob/rawhide/f/kitty.spec#_268 > > [5]: https://salsa.debian.org/debian/kitty/-/tree/debian/experimental?ref_type=heads > [6]: > https://salsa.debian.org/debian/kitty/-/blob/15a00c9b73f1008520c49f8c795f3bada3eed171/setup.py#L972 > [7]: > https://salsa.debian.org/debian/kitty/-/blob/15a00c9b73f1008520c49f8c795f3bada3eed171/debian/patches/0003-build-golang-components-with-dh-golang.patch > [8]: https://github.com/kovidgoyal/kitty/issues/5473 > [9]: > https://debian.pages.debian.net/-/kitty/-/jobs/4902978/artifacts/debian/output/lintian.html Let me know what you think about the discussion! Best, Nilesh
Attachment:
signature.asc
Description: PGP signature