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

waybar: init at 0.4.0 #54627

Merged
merged 2 commits into from Mar 20, 2019
Merged

waybar: init at 0.4.0 #54627

merged 2 commits into from Mar 20, 2019

Conversation

FlorianFranzen
Copy link
Contributor

@FlorianFranzen FlorianFranzen commented Jan 26, 2019

Motivation for this change

Initial addition of waybar.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

Lots of comments, but don’t worry. Mostly formatting.

pkgs/applications/misc/waybar/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/waybar/default.nix Show resolved Hide resolved
pkgs/applications/misc/waybar/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/waybar/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/waybar/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/waybar/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/waybar/default.nix Outdated Show resolved Hide resolved
@FlorianFranzen
Copy link
Contributor Author

FlorianFranzen commented Jan 28, 2019

@alyssais Update package file as requested, see rewritten commit.

@FlorianFranzen FlorianFranzen mentioned this pull request Jan 28, 2019
10 tasks
@FlorianFranzen FlorianFranzen changed the title waybar: init at 0.2.3 waybar: init at 0.3.0 Jan 30, 2019
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

Couple more style notes.

pkgs/applications/misc/waybar/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/waybar/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/waybar/default.nix Outdated Show resolved Hide resolved
@FlorianFranzen
Copy link
Contributor Author

@alyssais: Thanks for the feedback. Updated the expression and squashed it into one commit.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@FlorianFranzen
Copy link
Contributor Author

@alyssais Now trailing whitespace free.

@FlorianFranzen
Copy link
Contributor Author

Now also includes a NixOS module to run waybar as systemd service.

@primeos I am interested in implement sway's recommed systemd integration in NixOS. Any feedback to this module would be welcome, especially on how to pass the SWAYSOCK variable to waybay and others (without a call to systemctl --user import-environment ...).

@FlorianFranzen
Copy link
Contributor Author

@alyssais Is there anything else missing before we can get this merged?

@primeos
Copy link
Member

primeos commented Feb 19, 2019

@primeos I am interested in implement sway's recommed systemd integration in NixOS. Any feedback to this module would be welcome, especially on how to pass the SWAYSOCK variable to waybay and others (without a call to systemctl --user import-environment ...).

Sorry for my late response, I'm not a huge fan of systemd user units (at least ATM) and therefore also not that familiar with them.

There should be multiple ways to pass SWAYSOCK but I'm not sure which one is the best. It would be nice if SWAYSOCK could be inherited by having a dependency on sway.service but I assume this isn't the case. It should be possible to use systemctl --user import-environment SWAYSOCK from withing sway.service but that solution could get a bit complicated (doesn't seem like the worst idea though). Another approach would be to hard-code SWAYSOCK but this will become problematic if users need multiple Sway sessions (though that most likely won't work well with systemd anyway) and might cause other problems. Using systemd environment file generators (man systemd.environment-generator) should also be possible for a more dynamic approach.

But in any case it might be best to first check how other units are solving this problem.

@alyssais
Copy link
Member

@FlorianFranzen I'm happy to merge this once I see an ack of @primeos's comment from you. Let me know if you plan to change it further – I was about to merge it before and then a whole new module appeared!

@FlorianFranzen
Copy link
Contributor Author

In do not want to make waybar sway-only as it is not by design. That is why I made it depend on the graphical-session.target instead, which should be the correct and general way (see e.g. #22671).

@primeos Thank you for your feedback. As SWAYSOCK is needed inside a sway session for the IPC message client to function anyway, any systemd solution will have to take care of this as well. If I find some time to clean up my solution, I will make sure to open a pull request and let you know.

Hardcoding SWAYSOCK is not sensible solution as the name always contains the process id of sway (i.e. /run/user/1000/sway-ipc.1000.1547.sock).

@alyssais I have no plans to change this further, thank you for your help to get this merged.

@FlorianFranzen
Copy link
Contributor Author

Updated waybar to latest release.

@FlorianFranzen FlorianFranzen changed the title waybar: init at 0.3.0 waybar: init at 0.4.0 Mar 12, 2019
@primeos primeos mentioned this pull request Mar 14, 2019
22 tasks
@alyssais
Copy link
Member

alyssais commented Mar 17, 2019 via email

@FlorianFranzen
Copy link
Contributor Author

@alyssais: No problem, that was bound to happen. I rebased the pull request against the latest unstable and replaced all reference to sway-beta accordingly.

@alyssais alyssais merged commit 0cd7f32 into NixOS:master Mar 20, 2019
@alyssais
Copy link
Member

Thank you for your persistence!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants