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

Update soon-to-be incompatible dependencies #3315

Merged
merged 5 commits into from
May 18, 2024

Conversation

har7an
Copy link
Contributor

@har7an har7an commented May 1, 2024

We've had warnigns cropping up during builds for a while now, warning us about future incompatibilities in some of our dependencies. Earlier attempts at resolving those have failed, unfortunately, since these were transitive dependencies of packages that had no later releases at that time.

Meanwhile this has changed and this PR alleviates the issue by updating the packages termwiz and daemonize.

@har7an har7an changed the title Update yanked dependencies Update soon-to-be incompatible dependencies May 1, 2024
har7an added a commit to har7an/zellij that referenced this pull request May 1, 2024
@har7an
Copy link
Contributor Author

har7an commented May 1, 2024

Hey @imsnif,

here's a heads-up to let you know that I'm fiddling with the dependencies (again). This removes the "deprecation" warnings for our deps we've been receiving in builds for a while. This requires a little modification in zellij_server::lib for the MacOS builds (It's resolving a umask value as u16 on MacOS, where it's a u32 on Linux, apparently) and I can't test whether this is an actual problem due to a lack of MacOS PCs.

I'd suggest we merge this and in case any reports crop up by fellow developers on MacOS complaining that their builds stopped working, this might be the cause. Please let me know in case that happens, or if you object to any of this.

I'm afraid that this change will become necessary at least when we bump the rust toolchain version again, since the code will probably no longer work then. I would prefer to "debug" issues with these two processes separately, though.

@imsnif
Copy link
Member

imsnif commented May 2, 2024

Hey, thanks for doing this. I'd rather test these things before releasing them... Can we hold this off until after the next patch version (which should hopefully happen in the next few days)?

@imsnif
Copy link
Member

imsnif commented May 2, 2024

Right, version is out. So - maybe @tlinford or @jaeheonji can help with testing this on mac when they have the time?

Also - which dependencies did we end up updating? The diff is a little hard to read since I think things changed order a bit? Hope you can help me out.

@har7an
Copy link
Contributor Author

har7an commented May 2, 2024

Right, version is out.

Damn, "the next few days" went by pretty fast. :)

which dependencies did we end up updating?

Right, I sorted the dependencies in the last commit here. We updated:

  • termwiz from 0.20.0 to 0.22.0
  • daemonize from 0.4.1 to 0.5.0

What MacOS-people should probably be looking out for are files created by the server thread. I would assume that includes the magical socket in the runtime dir? But actually I'm not sure. If we find out that the server doesn't create any files at all, maybe we can even drop that line entirely from the server code.

har7an added a commit to har7an/zellij that referenced this pull request May 2, 2024
@imsnif
Copy link
Member

imsnif commented May 2, 2024

Hum... so - both of these dependency updates are breaking changes. That's okay, but I'd like for us to do some thorough manual testing in everything they touch.

For daemonize this is everything involving creating new sessions (namely creating a session, detaching from it and reattaching, attaching with multiple users, from multiple terminals, etc). For termwiz this is about STDIN parsing... I think making sure we can bind all common keys will do the trick.

Sorry, I know this is a lot - but we gotta be 100% here and these things are not trivial to test automatically.

@imsnif
Copy link
Member

imsnif commented May 2, 2024

Addendum: thinking about it, if all the keys in the default config work (plus some eccentricities such as the new Ctrl j and other things here: https://github.com/zellij-org/zellij/blob/main/zellij-utils/src/input/mod.rs#L77-L87 it should be fine)

@har7an
Copy link
Contributor Author

har7an commented May 2, 2024

@imsnif No need to apologize, I much prefer we test these things before others do. But you definitely have a better overview about what these things touch and how we test that, so thanks for the plan. ;)

So let's try to formalize that:

daemonize

  • creating sessions works
  • detaching and reattaching to sessions works
  • attaching with multiple users works
  • attaching from multiple terminals works

termwiz

  • Bind all the keys from the default config
  • Push all the keys from the default config to see they work
  • Bind Ctrl+j
  • Bind Ctrl+h

If you want something added, let me know. I'll start testing this weekend. Do you think different terminal emulators have an influence on the test results, or can I use an arbitrary terminal for that?

@imsnif
Copy link
Member

imsnif commented May 2, 2024

Looks great! About binding: we also need to test all the keys after binding them - this is where termwiz does its thing for us. So I guess basically: go into every mode, test all the alt keys (especially Alt + [ and ]), etc.

@jaeheonji
Copy link
Member

@imsnif I'll also test all key combinations on my Mac device based on what @har7an has outlined until tomorrow. (Thanks for the summary.)

@tlinford
Copy link
Contributor

tlinford commented May 3, 2024

From a quick test on mac, it seems to run fine. Detached and reattached to a session, tried out the welcome layout, all ok as far as I can tell.

Copy link
Member

@jaeheonji jaeheonji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked on my Mac that everything works fine for all keys. 🚀

@har7an
Copy link
Contributor Author

har7an commented May 15, 2024

Hey, sorry for the long delay, I ran the tests mentioned above on Linux and they all completed fine.

One thing I did notice is that, apparently, Ctrl-based keybindings aren't really handled different from their "non-Ctrl" counterparts? As in: It appears that zellij can't really differ between the two (say: a and Ctrl+a). I'll have to investigate this some more, but that issues also appears on 0.40.0 and 0.40.1, so it's unrelated to this change. I'll try to write a good config to reproduce this and run a bisect when I find a PC big enough to compile zellij quicker than mine can. :P

If nobody else has any objections to the changes here, I'll merge them on saturday.

to 0.22.0 which, according to [their changelog][1], doesn't introduce
any changes at all over the previously used 0.20.0. It does, however,
update some of its' dependencies allowing us to update the transitive
deps `nom v5.1.2` and `terminfo v0.7.3`, which have caused warnings
during build/installation for quite some time now.

[1]: https://github.com/wez/wezterm/blob/main/termwiz/CHANGELOG.md
to v0.5.0, which eliminates a future-compat warning that has been around
for a while now. It doesn't state changes in the Changelog that we
should be aware of and doesn't cause apparent breakage during builds
either.
to avoid type conversion issues on MacOS builds.
@har7an har7an merged commit 64a5ac0 into zellij-org:main May 18, 2024
6 checks passed
@har7an har7an deleted the deps/update-yanked branch May 18, 2024 12:12
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.

None yet

4 participants