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.

Sign up for GitHub

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

Jump to bottom

sway: Various improvements (package, module, and documentation) #121925

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

sway: Various improvements (package, module, and documentation) #121925

wants to merge 2 commits into from

Conversation

primeos
Copy link
Member

@primeos primeos commented May 6, 2021

Motivation for this change

Feedback is welcome but unfortunately I don't have that much time for this so concrete changes / actionable feedback would be ideal/preferred (discussions are fine too - I'd just like to avoid updating commits multiple times). It might also make sense to split this into individual PRs but let's see first if there's a need for that.

Note: This isn't tested yet - it's basically an RFC.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

primeos
primeos commented May 6, 2021
View reviewed changes
Copy link
Member Author

@primeos primeos left a comment

Choose a reason for hiding this comment

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

Added some quick notes (reasons for changes and things that might require a discussion).

nixos/modules/programs/sway.nix Outdated Show resolved Hide resolved
nixos/modules/programs/sway.nix Outdated Show resolved Hide resolved
nixos/modules/programs/sway.nix
@@ -87,20 +89,35 @@ in {
type = with types; listOf package;
default = with pkgs; [
swaylock swayidle alacritty dmenu
rxvt-unicode # For backward compatibility (old default terminal)
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if that requires a release notes entry. It's likely annoying if starting a terminal doesn't work anymore but Sway users will hopefully be skilled enough by figuring this out quickly (e.g. resorting to dmenu).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could add a wrapper that launchers alacritty but displays a warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that but not sure if it's a good idea / if a nice implementation is possible (would have to be a graphical warning and then it get's hacky quickly). Do you have something concrete in mind?

Copy link
Member

Choose a reason for hiding this comment

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

You could display a warning in the shell itself: alacritty -e sh -c 'echo WARN; exec $SHELL'

I don't know exactly how you'd make sure the real rxvt-unicode overrides this fake rxvt-unicode.

Copy link
Member Author

Choose a reason for hiding this comment

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

You could display a warning in the shell itself: alacritty -e sh -c 'echo WARN; exec $SHELL'

That looks and sound super hacky to me (haven't tried if it even works or if exec $SHELL resets the output).
Users could be very confused by this.
And, while I haven't thought about that yet, launching Alacritty for users that expect rxvt-unicode also sounds wrong.
Anyway, if someone ACKs that I'd be ok with that but to me it doesn't really sound like a good idea (the intentions are good but I feel like I'd be super confused).

I don't know exactly how you'd make sure the real rxvt-unicode overrides this fake rxvt-unicode.

There's the lowPrio function ( meta.priority) that should be able to handle that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please no, wofi on nixpkgs master segfaults when I run it.

Copy link
Member

Choose a reason for hiding this comment

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

Great, can you get a backtrace and open an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Either way we should add a release notes entry, so people will not be too surprised about this.

How does #127927 look?

Also, wouldn't it be sensible to go with wofi until we have wmenu?

Our programs.sway.extraPackages is "unopinionated", we're simply sticking to the upstream defaults (i.e. Drew's opinion / default configuration). dmenu certainly isn't ideal but is seems like there's still no good enough replacement for it: swaywm/sway#5112

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, can you get a backtrace and open an issue?

The opposite of great. I was under the impression that we don't push broken packages to solicit bug reports.

Copy link
Member

Choose a reason for hiding this comment

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

Great(, that this problem is so reproducible for you), it would probably help you and others if we could report this issue upstream, hopefully with a backtrace (coredumpctl debug).

nixos/modules/programs/sway.nix Outdated
Comment on lines 98 to 113
# To run Qt5 applications natively on Wayland:
qt5.qtwayland
# Status bars:
i3status i3status-rust waybar
# Notification daemon:
mako
# Screenshots
grim slurp sway-contrib.grimshot
# dmenu replacements:
bemenu rofi
# Redshift
gammastep wlsunset
# Alternative terminals:
foot termite
# Backlight management:
brightnessctl light
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it's highly debatable what to include here :o We could also leave this empty and maybe install qt5.qtwayland by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just an example I guess, so don't think this is controversial in any way.

pkgs/applications/window-managers/sway/wrapper.nix Outdated Show resolved Hide resolved
@primeos primeos mentioned this pull request May 11, 2021
Merged
10 tasks
@ehmry
Copy link
Contributor

ehmry commented May 12, 2021

FYI, rxvt-unicode and alacritty are either broken or too expensive to build on aarch64. I have to mask extraPackages in a hardware module because the drivers there only support wayland and the system is unbuildable with those two dependencies.

https://github.com/nix-community/hardware-mnt-reform/blob/241e8c22d994007204de709e58e0c836ae1ec868/flake.nix#L86

@primeos
Copy link
Member Author

primeos commented May 12, 2021

@ehmry thanks, that's good to know. I plan on dropping rxvt-unicode after the NixOS 21.05 branch-off as we shouldn't keep it around forever. alacritty should remain part of the default though as it's used by Sway's default configuration (which should work out of the box for new users). But we've intentionally used extraPackages for this so that it can be customized easily.

Anyway, if alacritty isn't build on Hydra for AArch64 that seems like something we should change. It already seems to be build at least for nixpkgs-unstable (https://hydra.nixos.org/job/nixpkgs/trunk/alacritty.aarch64-linux and nixos-20.09: https://hydra.nixos.org/job/nixos/release-20.09-aarch64/nixpkgs.alacritty.aarch64-linux) but I didn't find it in the nixos:trunk-combined jobset (nixos-unstable).

@ehmry
Copy link
Contributor

ehmry commented May 13, 2021

Maybe it will build again for awhile, but I'm wary of my system depending on anything written in rust.

@primeos
nixos/sway: Stop installing rxvt-unicode by default
a268b5a 
This was done only for backward compatibility and is not required by the
default configuration anymore. Unfortunately this might be a breaking
change for some users.
@primeos
nixos/sway: Update the extraPackages examples
dd97746
@primeos primeos mentioned this pull request May 27, 2021
Open
22 tasks
@stale
Copy link

stale bot commented Jan 6, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 6, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mweinelt mweinelt mweinelt left review comments

@ehmry ehmry ehmry left review comments

@Synthetica9 Synthetica9 Synthetica9 left review comments

@Mindavi Mindavi Mindavi left review comments

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@primeos @ehmry @mweinelt @Synthetica9 @Mindavi @wegank