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 [email protected] 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
signature.asc
Description: PGP signature
