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

fix: vt100 patch #734

Closed
wants to merge 1 commit into from
Closed

Conversation

jeevithakannan2
Copy link
Contributor

@jeevithakannan2 jeevithakannan2 commented Oct 2, 2024

Recommended way of doing this is mentioned in additional information

Type of Change

  • New feature
  • Bug fix
  • Documentation update
  • Refactoring
  • Hotfix
  • Security patch
  • UI/UX improvement

Description

  • Add vt100 patch

Testing

  • No more panics

Impact

  • Extra crate in the repo

Issues / other PRs related

Additional Information

  • Added the vt100 patch from Fix scrollback offset bugs doy/vt100-rust#11
  • I don't know if this is the right way to do it but the issue is fixed now.
  • @adamperkowski I think doing it this way can interfere with packaging. Please look into it !!
  • @ChrisTitusTech The correct way is to create a branch in linutil with only vt100 source code with the fixes and patching the vt100 in linutil cargo.toml like this
[patch.crates-io]
vt100 = { git = "https://github.com/ChrisTitusTech/linutil", branch = "vt100" }
  • Rather than a branch a new repo would be good as cargo does not support shadow cloning ( only in nightly ), this will clone the entire history of linutil just for one crate vt100.
  • A separate repository would be better if we don't want the whole linutil to be cloned for a single crate.
  • If you think that is too much of a hassle, you could leave the whole bloat of the another crate in here.
  • @ChrisTitusTech please go through all the above available solutions and decide.

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

@adamperkowski
Copy link
Collaborator

adamperkowski commented Oct 2, 2024

This has been already discussed on Linutil's Discord server, summing it up here:

Obviously, adding 4k+ lines of someone else's code (without any attribution other than MIT license included btw) is a very bad idea.

  1. Repo & package size goes 🚀🚀🚀
  2. Basically unmaintainable
  3. We'd literally have 4k+ lines of code we didn't write, don't understand and is not even needed that much.

With this solution, we think Linutil should be moved to an organisation and a fork of vt100 should be created (and possibly published on crates.io) in a separate repo.

But that's 100% @ChrisTitusTech's decision. I am strongly against doing this in a branch.

@jeevithakannan2
Copy link
Contributor Author

jeevithakannan2 commented Oct 2, 2024

I agree with @adamperkowski . This PR is just to let you know the possible solutions we have, it is your @ChrisTitusTech decision

@cartercanedy
Copy link
Contributor

cartercanedy commented Oct 2, 2024

  1. Repo & package size goes 🚀🚀🚀

Repo yes, but considering that all rust source is effectively statically linked, probably not going to affect binary size in release

Still, I agree, v bad idea to inline here

But... I do have half a mind to fork myself and merge some of the open PRs from doy/vt100-rust since it seems like it'd benefit more than just us

@adamperkowski
Copy link
Collaborator

Repo yes, but considering that all rust source is effectively statically linked, probably not going to affect binary size in release

Never said i's going to affect binary sizes, no.
But package sizes, yes. This one.

But... I do have half a mind to fork myself and merge some of the open PRs from doy/vt100-rust since it seems like it'd benefit more than just us

That's why I proposed creating a separate repository :))

@cartercanedy
Copy link
Contributor

With this solution, we think Linutil should be moved to an organisation and a fork of vt100 should be created (and possibly published on crates.io) in a separate repo.

Just read this :))

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

Successfully merging this pull request may close these issues.

panic when scrolling
3 participants