search

darkhz / invidtui

A TUI based Invidious client
MIT License
191 stars 6 forks source link

Attempt to merge darkhz/tview with upstream rivo/tview #35

Closed Maytha8 closed 8 months ago

Maytha8 commented 8 months ago

invidtui depends on darkhz/tview, which is a fork of rivo/tview with a few changes. It would be beneficial if you open a PR and/or contact the upstream author to get the changes merged upstream.

I am aware of rivo/tview#703, but I think it would be a good idea to try and merge more of your work into rivo/tview.

darkhz commented 8 months ago

The thing is, my commits in the fork are for specific use-cases, and may not scale to cover a broader set of other use-cases by other users of the main repo. As such, I am hesitant to create PRs for the my commits in the main repo. I am however, syncing my fork when updates are issued to rivo/tview.

For example, the SetSelectorWrap property does not properly wrap a cell when borders are enabled for the table and tablecells. Since I don't enable borders for my tables generally, I haven't covered this particular thing for the feature.

Maytha8 commented 8 months ago

Thanks for the quick repsonse!

I still think it's a good idea to at least open feature requests at rivo/tview if not PRs, since your changes are reasonable and can be beneficial to others.

I'm trying to package all the dependencies of invidtui for Debian, and they prefer forks are merged upstream rather than packaged individually.

AFAICS regarding SetSelectorWrap, that's something that can be fixed and polished for merging.

darkhz commented 8 months ago

Thank you for starting the packaging process for Debian. Can you share your package script, so that I can review it as well? Also, are you setting up the package script to build invidtui from scratch? Wouldn't downloading the release binaries suffice?

Maytha8 commented 8 months ago

@alexmyczko is packaging invidtui, I'm helping him out with packaging the dependencies.

You can see the invidtui packaging on Salsa (Debian's GitLab instance): https://salsa.debian.org/go-team/packages/invidtui

In Debian, binary packages must be accompanied by source packages (which contain upstream's code + Debian-specific packaging stuff inside the debian/ directory). You must be able to build a Debian package using only the source package and other packages in Debian, and there must be no reliance on an internet connection (it must work offline). If you'd like, I can find some extracts from the Debian Policy Manual to back this up.

Maytha8 commented 8 months ago

Note that on the Salsa repo, there's been a mistake, you'll need to switch to master branch to see the actual work.

The "package script" is debian/rules. Right now, it's pretty empty as it uses other tools to do the work. Essentially, the helpers run go install github.com/darkhz/invidtui/... using Debian packages (in GOPATH mode) rather than downloading the deps.

darkhz commented 8 months ago

In Debian, binary packages must be accompanied by source packages

Ah, that is quite a limitation.

I am on the master branch, currently looking into the control and rules scripts.

darkhz commented 8 months ago

As for merging the fork upstream, there will be no guarantees. rivo also does not merge PRs quickly, and given the amount of existing PRs, it will take a lot of time to review and merge. There could be an alternate strategy, I am looking into the packaging rules for debian as well.

I will consider submitting feature requests, but even those will take time to be implemented, tested and merged.

darkhz commented 8 months ago

they prefer forks are merged upstream rather than packaged individually.

@Maytha8 can you link the discussion/instructions where they mention this requirement?

Maytha8 commented 8 months ago

@Maytha8 can you link the discussion/instructions where they mention this requirement?

From IRC today, #debian-golang on OFTC:

[2:03:10 pm] <pabs> gargantua_kerr, maytha8: I think packaging the fork would be better than vendoring
[2:03:46 pm] <pabs> (upstreaming the patches is of course the best option)

A while back from the debian-go mailing list: https://lists.debian.org/debian-go/2023/10/msg00047.html

Maytha8 commented 8 months ago

In Debian, binary packages must be accompanied by source packages

Ah, that is quite a limitation.

Not really a limitation, more of a perk as it offers a consistent way to build packages. It offers a consistent way to download the source code of a given package and build it. i.e. The same command can be used to download and/or build any Debian package.

As for merging the fork upstream, there will be no guarantees. rivo also does not merge PRs quickly, and given the amount of existing PRs, it will take a lot of time to review and merge. There could be an alternate strategy, I am looking into the packaging rules for debian as well.

For now, I've packaged darkhz/tview.

darkhz commented 8 months ago

Not really a limitation, more of a perk as it offers a consistent way to build packages. It offers a consistent way to download the source code of a given package and build it. i.e. The same command can be used to download and/or build any Debian package.

Yes, good point.

For now, I've packaged darkhz/tview.

Very good, thank you. I assume the build process will be smooth now, since all the dependencies are configured.

alexmyczko commented 8 months ago

looks very good, http://sid.ethz.ch/debian/go/

darkhz commented 8 months ago

Again, my thanks to both of you for packaging for debian, I know it was quite a task to do it. This issue will be closed for now.