-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
sway: 1.6.1 -> 1.7 #151902
sway: 1.6.1 -> 1.7 #151902
Conversation
I'm this close 🤏🏻 to suggesting we just add a `sway_default_terminal`
symlink and patch the config to use that instead of keeping all old
terminals around for ~5 releases
…On Thu, 23 Dec 2021, 20:38 ofborg[bot], ***@***.***> wrote:
@ofborg <https://github.com/ofborg>[bot] requested your review on: #151902
<#151902> sway: 1.6.1 -> 1.7.
—
Reply to this email directly, view it on GitHub
<#151902 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABV7PJ25OTSPSDZWFYOUUZTUSN3BXANCNFSM5KVLSJNQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
vscod{e,ium} with ozone platform wayland also seem broken, but I assume that's a wlroots thing rather than a Sway thing |
@Synthetica9 thanks for the testing and feedback! :) Unfortunately I'm currently quite behind with testing as I wanted for Hydra to build more of staging-next but it's progressing slowly / will take a while (I might switch to cherry-picking for now).
That would also be an option. But then again I'm hoping that this'll be the last change of the default terminal emulator for a while. Last time we dropped Given that
I've added it to the TODOs, thanks. The full (debug) output should hopefully contain enough information to find a fix/workaround.
Oh, that doesn't sound good... :o Hopefully it doesn't affect the current Chromium as well. Unfortunately Ozone/Wayland is still far from stable (I forgot the official status but IIRC Igalia still considers it alpha or beta). Edit: VM test output
Sway (normal output):
|
Chromium also broke, definitely a blocker then: $ chromium --verbose --enable-features=UseOzonePlatform --ozone-platf
orm=wayland
interface 'wl_output' has no event 4
[1229/184110.716325:ERROR:process_memory_range.cc(75)] read out of range
fish: Job 1, 'chromium --verbose --enable-fea…' terminated by signal SIGTRAP (Trace or breakpoint trap)
$ rwhich chromium
/nix/store/ds5pnbnjhdb96azyisgmkikhw0g0c65q-chromium-96.0.4664.110/bin/chromium Currently looking if I can get the master of another wlroots compositor to work to confirm this is indeed a wlroots bug. |
This is a Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1279574 (patch) |
Yikes, this might mean Electron apps may stay broken for a while (especially since we don't build most of those)... |
I've "fixed" the VM test by switching to the new Pixman renderer. It's more of a workaround than a fix but we already had to use software rendering before and this should work more reliably for the VM test. I currently lack time for a more thorough analysis but from a quick test wlroot's TinyWL doesn't seem affected.
That is indeed really unfortunate... :o |
@ofborg test sway 2 |
Updated the test to use foot.
Would also be possible to fix this from wlroots perhaps? Honestly, I wouldn't feel comfortable upgrading (the default version of) Sway to 1.7 if something I think quite a lot of users use would knowingly break without a good fix... I know I would be staying on Sway 1.6 (for now) if the full version of 1.7 were to release today |
Sounds perfect for unstable. I'd really like to use it personally due to some of the fixes. It doesn't sound like I'll be affected by any of the unfortunate chromium issues. Let me see if I can figure out how to apply patches 😉 Gotta get DRM leasing for VR headsets. 🤞 |
While nixos unstable is a rolling-release distro, in my experience it is also one of the most stable rolling-release distros out there. I'd like to avoid knowingly breaking things if I can avoid it.
Haha yeah I guess I'm biased towards the chromium issue because I use a lot of electron-based apps and have a mixed-scaling setup, which isn't great with XWayland so I use as many wayland-native apps as I can get away with :P
The problem is that we don't build a lot of the electron apps ourselves, but grab pre-built binaries for them, so it would be difficult to patch those. |
@Synthetica9 thanks for 0860d0d but can you please drop 5ec0950? It just makes working on this PR unnecessarily annoying (I intend to squash all commits at the end btw - assuming it doesn't really help to keep some changes separate - therefore this PR should also only contain the relevant changes for the update) and doesn't belong in this PR ("destroys" the diff; feel free to reformat it after that).
+1, the name nixos-unstable is misleading, unfortunately. However, in this case I'm not sure yet how to proceed. One possibility would be to add In any case the goal should be to (eventually) update Sway to version 1.7 by default.
Agreed, the current status sucks for Electron users... :o
I don't think they could apply a good workaround (without downgrading the supported I think the better option would be to try to get the Chromium patches backported to older releases and Electron (ideally that would already be happening as this should affect other Wayland compositors soon as well, I guess). |
✅
Might honestly be the best option in this case, re-evaluate once all or most electron packages have the relevant patch.
What's the reasoning behind this?
Yeah, I fully understand the sentiment of the wlroots people to not want to do such a workaround, but but it leaves us in a difficult situation downstream. |
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.
What's the reasoning behind this? (package option)
#50476 (it wasn't necessary so far) but it seems like I forgot about the last update: #50476 (comment)
So I guess it could make sense to add such an option now (too choose between Sway 1.6 and 1.7 - apart from that there's probably little use for it as overlays can already be used as an alternative and AFAIK there are no relevant Sway alternatives/forks).
Yeah, I fully understand the sentiment of the wlroots people to not want to do such a workaround, but but it leaves us in a difficult situation downstream.
Yes, unfortunately. Regarding Chromium: I tested the fixes with M98 (#153633) and it worked but I got a few other errors that broke hardware acceleration (but I need to check if that's a regression or not as I haven't used Chromium on that system for quite a while...).
Might be worth trying with |
Crashes with a similar error:
|
I think Pixman should be fine for now. We needed For other renderers we should look into getting proper hardware acceleration to work: Lines 20 to 23 in 02a162f
I wouldn't consider it a high-priority TODO but feel free to have a look at it if someone has time and motivation. IIRC it might at least require changes to Mesa as well since we currently don't even build VirGL by default (I enabled it once but it still wasn't enough - maybe that's changed though). For Vulkan there's also Venus now (https://docs.mesa3d.org/drivers/venus.html) but it requires Vulkan support from the host driver (so not appropriate for our VM tests; plus there are a lot of other requirements that we cannot satisfy). In the end hardware acceleration might not be possible on a headless setup yet. |
Updating to a newer wlroots' version will also mean we will need this patch in all chromium based applications. Otherwise, due to the new protocol version, all these apps will crash with The patch was included in |
Yes, we already noticed and discussed it here (s. above). Chromium in Nixpkgs is also patched since today (#155203).
In the meantime I thought a bit more about this. I'm now for defaulting Sway to version 1.7. This isn't a Sway/wlroots bug, Chromium is now patched, and Electron apps would have to be used via XWayland in the meantime (IMO this is fair since Ozone/Wayland is still in alpha (or beta?)). It's certainly not ideal but IMO by far not enough to hold the update back. If deemed necessary we could add a |
This was also discussed above, starting at #151902 (comment)
Thank you for keeping an eye on this, I hope this will trickle down faster than we expect! |
@ofborg test sway |
For apps which bundle their own outdated electron versions, it's usually possible to use an alternative electron version. Could consider doing this for apps which stop working with this update. Example arch build doing this for discord https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=discord_arch_electron |
1.7 final was released yesterday |
Release notes: https://github.com/swaywm/sway/releases/tag/1.7 Notable (backward incompatible) changes: - The default terminal changed from Alacritty to foot Known issues: - `swaynag` will crash when Sway 1.6.1 is still running while the Nix package (and thus `swaynag`) is already updated to version 1.7. - The experimental Ozone/Wayland support of Electron apps will be broken for a while. Electron version 17 should work but the Chromium fixes haven't yet been backported to Electron version 16. NixOS module: programs.sway.extraPackages: The "alacritty" package was replaced with "foot". VM test: We switched from the OpenGL ES 2.0 renderer to Pixman. The terminal was also changed to foot but Alacritty is still used for the XWayland test (since foot doesn't support X11). Co-authored-by: Patrick Hilhorst <[email protected]>
Ok, so from my point of view this is ready to be merged as is. I'm running RC3 on my main setup since it was released and didn't notice any new issues so far (apart from not being able to use the Wayland support of Electron apps (but luckily I don't need any Electron apps anymore) - that isn't a Sway bug though and there's still XWayland or nested Wayland compositors). The diff also looks good to me (a few additional changes but still a clean diff). So I suggest we merge this :) Or are there any objections left? |
Do we want to keep 1.6 around for the time being to accommodate Electron
users?
…On Sun, 23 Jan 2022, 19:46 Michael Weiss, ***@***.***> wrote:
Ok, so from my point of view this is ready to be merged as is. I'm running
RC3 on my main setup since it was released and didn't notice any new issues
so far (apart from not being able to use the Wayland support of Electron
apps (but luckily I don't need any Electron apps anymore) - that isn't a
Sway bug though and there's still XWayland or nested Wayland compositors).
The diff also looks good to me (a few additional changes but still a clean
diff). So I suggest we merge this :) Or are there any objections left?
—
Reply to this email directly, view it on GitHub
<#151902 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABV7PJZYWS3J3N2XD5QJOY3UXREJHANCNFSM5KVLSJNQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
That's the question. I've written about it in #151902 (comment) but there were not replies. I don't feel that it's necessary (we already provide override mechanics) and if we'd add one we'd also need Anyway, I'm not objecting if someone thinks that having |
Personally, I'm waiting for this to land. Electron works under XWayland, and there are other override mechanisms, so the only argument in favor of keeping sway_1_6 is to enable users to choose a third workaround mechanism as opposed to one of two others. Given the added maintenance cost, I can't find the value. |
Okay, feel free to merge then. I'm opening a tracking issue for the Chromium situation |
Agreed, my impression is having multiple explicit versions of packages really should be limited to packages with explicit versions that are often pinned such as programming languages/runtimes and databases i.e it's a configuration supported by upstream. If someone really needs to hold back a package completely there are methods for using overlays in their Nix config. I would rather see those methods become better documented and used more frequently than for the nixpkgs repository to accumulate more and more workarounds. |
Ok, thanks for the quick feedback. Let's merge this then :) 🚀 I also want to thank everyone who helped via reviews/feedback, etc. (i.e. all participants of this PR). |
@Synthetica9 What GPU/GPU driver are you using? I'm an Vega 8/amdgpu and haven't had any regressions. Although I've been using chromium and several Electron apps like VScode, Electronmail, and Freetube and they all seem to work so they must all be defaulting to XWayland. The only thing I have noticed is that the new |
This is in the QEMU test driver, on an Intel GPU. |
Motivation for this change
TODOs:
Known issues:
swaynag
is from 1.7 it will crash with:Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes