Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Maytha8 opened this issue Jan 2, 2024 · 13 comments
Closed

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

Maytha8 opened this issue Jan 2, 2024 · 13 comments

Comments

@Maytha8
Copy link

Maytha8 commented Jan 2, 2024

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
Copy link
Owner

darkhz commented Jan 2, 2024

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
Copy link
Author

Maytha8 commented Jan 2, 2024

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
Copy link
Owner

darkhz commented Jan 2, 2024

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
Copy link
Author

Maytha8 commented Jan 2, 2024

@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
Copy link
Author

Maytha8 commented Jan 2, 2024

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
Copy link
Owner

darkhz commented Jan 2, 2024

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
Copy link
Owner

darkhz commented Jan 2, 2024

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
Copy link
Owner

darkhz commented Jan 2, 2024

they prefer forks are merged upstream rather than packaged individually.

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

@Maytha8
Copy link
Author

Maytha8 commented Jan 2, 2024

@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
Copy link
Author

Maytha8 commented Jan 2, 2024

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
Copy link
Owner

darkhz commented Jan 2, 2024

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
Copy link

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

@darkhz
Copy link
Owner

darkhz commented Jan 4, 2024

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.

@darkhz darkhz closed this as completed Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants