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

nixos/waybar: use upstream service file #242728

Closed
wants to merge 1 commit into from
Closed

nixos/waybar: use upstream service file #242728

wants to merge 1 commit into from

Conversation

m-bdf
Copy link
Contributor

@m-bdf m-bdf commented Jul 10, 2023

Description of changes

Use the more complete upstream waybar.service file instead of manually re-implementing it.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 10, 2023
@rodrgz
Copy link
Member

rodrgz commented Jul 17, 2023

Result of nixpkgs-review pr 242728 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
1 package built:
  • waybar

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2441

SuperSandro2000
SuperSandro2000 reviewed Jul 21, 2023
View reviewed changes
nixos/modules/programs/wayland/waybar.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
@m-bdf m-bdf force-pushed the waybar-use-upstream-service-file branch from 3d27246 to 2869aad Compare July 30, 2023 15:43
@m-bdf m-bdf requested a review from SuperSandro2000 July 30, 2023 16:33
@m-bdf m-bdf force-pushed the waybar-use-upstream-service-file branch 3 times, most recently from 7c040b8 to 8be1870 Compare October 17, 2023 11:37
@ofborg ofborg bot requested review from khaneliman and jtbx October 17, 2023 12:46
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 labels Oct 17, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2781

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@m-bdf m-bdf force-pushed the waybar-use-upstream-service-file branch from 8be1870 to 4e238a9 Compare April 3, 2024 19:42
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 3, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024
@m-bdf m-bdf force-pushed the waybar-use-upstream-service-file branch from 4e238a9 to bfe25ca Compare May 22, 2024 15:41
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4149

minijackson
minijackson approved these changes Jun 26, 2024
View reviewed changes
Copy link
Member

@minijackson minijackson left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Result of nixpkgs-review pr 242728 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
1 package built:
  • waybar

khaneliman
khaneliman requested changes Jun 26, 2024
View reviewed changes
pkgs/by-name/wa/waybar/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/wa/waybar/package.nix
@@ -163,13 +166,15 @@ stdenv.mkDerivation (finalAttrs: {
"pulseaudio" = pulseSupport;
"rfkill" = rfkillSupport;
"sndio" = sndioSupport;
"systemd" = false;
"systemd" = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably follow the same theme as the other flags if someone wants to build without systemd dependency

Copy link
Contributor Author

@m-bdf m-bdf Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This option only enables copying waybar.service to the correct location (PKG_CONFIG_SYSTEMD_SYSTEMDUSERUNITDIR) during the build, nothing else.

The result waybar binary remains identical as the systemd dependency is never used at runtime, so I don't really see a need to allow disabling it. I even believe a systemdSupport option would actually confuse people by making them think the waybar binary can depend on and use the systemd dependency at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what buildInputs (i.e. depsHostTarget) implies, that it's a runtime linkage.
If it's not used at runtime why would it be there?

@m-bdf m-bdf force-pushed the waybar-use-upstream-service-file branch from bfe25ca to ba0f895 Compare August 5, 2024 09:08
@ofborg ofborg bot requested review from khaneliman and minijackson August 5, 2024 09:48
@khaneliman khaneliman mentioned this pull request Sep 10, 2024
Merged
13 tasks
@m-bdf m-bdf closed this Sep 22, 2024
@m-bdf m-bdf deleted the waybar-use-upstream-service-file branch September 22, 2024 10:26
@m-bdf m-bdf restored the waybar-use-upstream-service-file branch September 28, 2024 13:25
@m-bdf m-bdf deleted the waybar-use-upstream-service-file branch September 28, 2024 14:00
@khaneliman khaneliman mentioned this pull request Sep 28, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eclairevoyant eclairevoyant eclairevoyant left review comments

@rodrgz rodrgz Awaiting requested review from rodrgz

@Synthetica9 Synthetica9 Awaiting requested review from Synthetica9

@FlorianFranzen FlorianFranzen Awaiting requested review from FlorianFranzen

@lovesegfault lovesegfault Awaiting requested review from lovesegfault

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@jtbx jtbx Awaiting requested review from jtbx

@khaneliman khaneliman Awaiting requested review from khaneliman

@minijackson minijackson Awaiting requested review from minijackson

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@m-bdf @rodrgz @nixos-discourse @eclairevoyant @minijackson @khaneliman @SuperSandro2000 @wegank