jesseduffield / lazygit-debian

Debian package for lazygit
Other
14 stars 0 forks source link

Packaging github.com/aybabtme/humanlog [#1055740] #31

Closed jmkim closed 10 months ago

anoopmsivadas commented 1 year ago

Hi @jmkim , I have one doubt regarding the humanlog packaging. Lazygit uses Scanner API from an old version(v0.4.1) of humanlog. Humanlog no longer provides this function. So how can we handle this? we cannot package an old version for debian, right?

jmkim commented 1 year ago

Hi @anoopmsivadas,

Thanks for your careful work \o/

If in that case, we should ask @jesseduffield to update the lazygit for adopting the lastest version of humanlog.

@anoopmsivadas, to the lazygit repo, suppose:

How do you think?

anoopmsivadas commented 1 year ago

@jmkim Then I'll try to fix this issue and raise a PR hopefully :) .

jesseduffield commented 1 year ago

Thanks @anoopmsivadas !

anoopmsivadas commented 1 year ago

The function signature of humanlog Scanner changed from func Scanner(src io.Reader, dst io.Writer, opts *HandlerOptions) error to func Scan(ctx context.Context, src io.Reader, sink sink.Sink, opts *HandlerOptions) error. Now it depends on two private packages accessible only to humanlog module. So I don't think a dependency upgrade would be possible.

anoopmsivadas commented 1 year ago

Hi @jmkim , @jesseduffield . Should we look for an alternative :eyes: or integrate the old humanlog to lazygit :grimacing: !!

anoopmsivadas commented 1 year ago

Hi @jmkim, Antoine (Author of Humanlog) is fine with moving these internal dependency to public. But we will have to package/upgrade few more modules go.mod.

WeepingClown13 commented 11 months ago

FYI: ITP: humanlogio-humanlog and ITP: connectrpc-connect

anoopmsivadas commented 11 months ago

@WeepingClown13 oh its cool. I can make a few more changes in upstream to ease the process. (have to find something else to package too 😅 )

WeepingClown13 commented 11 months ago

>have to find something else to package too

I can help out with this as long as it is not complex ig. Which package?

Edit: I misread "something" as "someone" xD (offer still counts though!)

jmkim commented 11 months ago

Hey @anoopmsivadas, I saw today Maytham filed ITP for this package. I now figured out that you didn't send ITP yet.

I am not sure Maytham already discussed with you about filing the ITP. (Because Maytham's purpose is for lazygit, I am guessing he already discussed with you privately?)

In Debian, ITP exposes the intention officially, so Maytham should take this package.

As I know, although you didn't send ITP yet, you are still digging the issue deeply on this package. I think you have to discuss with Maytham if you still have an interest for wrapping up this package.

anoopmsivadas commented 11 months ago

I already made some changes in upstream, was planning to send a PR for lazygit and then start from the humanlog dependencies. Since ITP is already sent by someone else they can package it. I'll make the changes in lazygit once the changes I made in humanlog releases. I might work on the humanlog dependencies as time permits. and will start with an ITP next time :)

Also please let me know if you need help with any other go packages :)

Maytha8 commented 11 months ago

Hi there! I'm the guy that filed the ITP.

Since ITP is already sent by someone else they can package it.

If you'd like me to package it, I'm fine with that. I can also reassign the ITP to you, which is perfectly fine.

Maytha8 commented 11 months ago

Regarding the humanlog dependencies, I've found that all of them have packages except for connectrpc.com/connect, which I can package if you'd like me to (I've already filed an ITP for this one as well.)

Note: github.com/bufbuild/connect-go is listed in the go.mod for humanlog, but it's been moved.

jmkim commented 11 months ago

I've found that all of them have packages except for connectrpc.com/connect

I added the issue for this: #41 \o/

jmkim commented 11 months ago

ITP : https://bugs.debian.org/1055740

Maytha8 commented 11 months ago

@anoopmsivadas I'm happy for you to continue your packaging efforts if you'd like. If so, I will transfer the ITP over to you and transfer the packaging repo https://salsa.debian.org/go-team/packages/golang-github-humanlogio-humanlog over to you (it's a clean slate at the moment). I'm also happy to co-maintain the package with you if you'd like. Let me know what you think. :)

Maytha8 commented 11 months ago

I've got a finished package at this Salsa repo for golang-github-humanlogio-humanlog, but I haven't send an RFS yet since I'm awaiting it's dependencies to be uploaded to Debian (#41 and #42).

WeepingClown13 commented 11 months ago

@Maytha8 maybe it is better to send the rfs and mention that you need the dependencies to be uploaded first, as it usually takes some time to get things running in the go team (I mean, I have 4 pending rfs there for a while now xD). Or maybe you can ask in the go team IRC channel whether that is a good idea.

jmkim commented 11 months ago

I've got a finished package at this Salsa repo

I cannot see the code o/

Maytha8 commented 11 months ago

I cannot see the code o/

@jmkim sorry I meant https://salsa.debian.org/go-team/packages/golang-github-humanlogio-humanlog/ (forgot the github- part of the link).

Maytha8 commented 11 months ago

@Maytha8 maybe it is better to send the rfs and mention that you need the dependencies to be uploaded first, as it usually takes some time to get things running in the go team (I mean, I have 4 pending rfs there for a while now xD). Or maybe you can ask in the go team IRC channel whether that is a good idea.

@WeepingClown13 I don't think there's any point in sending out an RFS for this package if the dependencies aren't there yet, since it can't be uploaded anyways without the deps being available. Let me know if you disagree.

I'm also in contact with Nilesh Patra (same person who reviewed your RFS) who's very active and will hopefully sponsor the packages soon if and when he has time.

WeepingClown13 commented 11 months ago

@Maytha8 haha I was simply hoping that it could speed up the process, anything you think would work is fine :) Hopefully Nilesh can take care of it. I hope he can take care of my rfs as well, I should've hinted I was about to do it when I met him two weeks ago xD He seems really busy these days.

Maytha8 commented 11 months ago

Nilesh has uploaded #41 but not #42, for the following reason (below is an extract from my email reply):

On Fri, 2023-11-17 at 14:11 +0530, Nilesh Patra wrote:

This looks like really old code, and there's golang-github-go-logfmt-logfmt in debian already.

Can the code in the target package be patched to use go-logfmt or is it using specific features from kr-logfmt?

The target package (golang-github-humanlogio-humanlog[1][2]) uses the Handler type from kr-logfmt for its own Handler type (handler.go), and this is the only occurrence of kr-logfmt in the code.

golang-github-go-logfmt-logfmt doesn't have this Handler type that's being used, and humanlog already uses go-logfmt as much as it can

I've made an issue upstream at humanlogio/humanlog#71 regarding this ~~and I've patched out the Handler type completely from humanlog for now.~~

Maytha8 commented 11 months ago

Both deps #41 and #42 have been uploaded

Maytha8 commented 11 months ago

RFS sent

Maytha8 commented 11 months ago

Delayed due to licensing issue, see humanlogio/api#1 for more info.

Maytha8 commented 11 months ago

Licensing issue fixed after an email to Antoine. Now in NEW queue, awaiting ftpmasters' approval.

Maytha8 commented 10 months ago

@jmkim Accepted! Can you please close this as done?