-
-
Notifications
You must be signed in to change notification settings - Fork 610
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
Conversation
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 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. |
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)? |
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. |
Damn, "the next few days" went by pretty fast. :)
Right, I sorted the dependencies in the last commit here. We updated:
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. |
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. |
Addendum: thinking about it, if all the keys in the default config work (plus some eccentricities such as the new |
@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
termwiz
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? |
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 |
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. |
There was a problem hiding this 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. 🚀
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.
75a7b4b
to
f7d149d
Compare
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
anddaemonize
.