search

ChrisTitusTech / linutil

Chris Titus Tech's Linux Toolbox - Linutil is a distro-agnostic toolbox designed to simplify everyday Linux tasks.
https://christitus.com
MIT License
2.77k stars 227 forks source link

*IMPORTANT* packaged builds not working with `patch.crates-io` #948

Open solomoncyj opened 2 days ago

solomoncyj commented 2 days ago

https://copr.fedorainfracloud.org/coprs/build/8262222

as we are required to use system packages, we cannot use the crate provided by [patch.crates-io]

(for some reson open suse goes through, thodue to their vendoring policy https://build.opensuse.org/package/show/utilities/linutil)

adamperkowski commented 1 day ago

cargo publish fails too

adamperkowski commented 1 day ago

It's impossible to include a patched dep in a package.

https://github.com/rust-lang/cargo/issues/13222

adamperkowski commented 1 day ago

The one way to fix this is to somehow convince the owner of tui-term to use vt100-ctt instead of vt100 or just for tui-term and maintain it with vt100-ctt.

https://crates.io/crates/vt100-ctt https://github.com/a-kenji/tui-term

solomoncyj commented 1 day ago

why do you need to use vt100-ctt?

solomoncyj commented 1 day ago

(for some reson open suse goes through, thodue to their vendoring policy https://build.opensuse.org/package/show/utilities/linutil)

jeevithakannan2 commented 1 day ago

why do you need to use vt100-ctt?

The original vt100 had a issue where it would panic if scroll is used when running a command. vt100-ctt patches that bug.

a-kenji commented 4 hours ago

Hello! Maintainer of https://github.com/a-kenji/tui-term here.

I have often thought about maintaining a fork of vt100, and I guess the need for many people is there. Seeing there are multiple forks on crates.io currently.

The one way to fix this is to somehow convince the owner of tui-term to use vt100-ctt instead of vt100 or just for tui-term and maintain it with vt100-ctt

While I am not opposed in general to having in-tree support of a vt100 fork, there is overhead associated with that. I am not comfortable at the current time to add the extra maintenance cost - I want to check out further development of the fork first. And I just now became aware of the vt100-ctt fork.

Since I am well aware of the current situation though - the vt100 backend is already pluggable and a fork of tui-term is as far as I can tell not needed.

You would likely want to disable the default vt100 feature. You can implement the following out of tree: https://github.com/a-kenji/tui-term/blob/db1fc6762fc99706e522338ec073c3fef222a241/src/vt100_imp.rs, so in essence you need to impl Screen and Cell from tui-term. Here you can see how vercel is doing that with their fork: https://github.com/vercel/turborepo/blob/ac1eab9105534ac9c43fb6c403b651097375a3c3/crates/turborepo-vt100/src/tui_term.rs.

jeevithakannan2 commented 4 hours ago

@a-kenji I created a PR in tui-term regarding the vt100-ctt. The fork just applies the patch to the scroll panic bug. Nothing more.

a-kenji commented 4 hours ago

I created a PR in tui-term regarding the vt100-ctt. The fork just applies the patch to the scroll panic bug. Nothing more.

@jeevithakannan2 I saw that. Applying your patch and releasing a new version would break all users of tui-term that didn't switch to the vt100-ctt fork. I hope you can understand why this is not an option for me.

I also think I showed a reasonable short term solution for the meantime.

jeevithakannan2 commented 3 hours ago

I created a PR in tui-term regarding the vt100-ctt. The fork just applies the patch to the scroll panic bug. Nothing more.

@jeevithakannan2 I saw that. Applying your patch and releasing a new version would break all users of tui-term that didnt't switch to the vt100-ctt fork. I hope you can understand why this is not an option for me.

I also think I showed a reasonable short term solution for the meantime.

Yeah I get it. Considering the fact that no one have created an issue regarding the scroll in tui-term I think it's best the way it is. Maybe in future if tui-term gets used by more people you need to consider adding vt100-ctt or by that time a new fork that is actively maintained rather than vt100 may pop up