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

nixos/shairport-sync: fix dbus support #276693

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vs49688
Copy link
Contributor

@vs49688 vs49688 commented Dec 25, 2023

Description of changes

Adds a policy (based off upstream's) that allows root and the configured user to own/use the main service and MPRIS interface.

Note that the upstream policies cannot be directly used because we allow configuring the user.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 👍 reaction to pull requests you find important.

Adds a policy (based off upstream's) that allows root and the configured
user to own/use the main service and MPRIS interface.

Note that the upstream policies cannot be directly used because we allow
configuring the user.
@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 Dec 25, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 25, 2023
@vs49688 vs49688 requested a review from fpletz December 26, 2023 13:27
@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/3159

@@ -6,6 +6,37 @@ let

cfg = config.services.shairport-sync;

# This is based off the upstream versions, but they can't be used directly because
Copy link
Member

Choose a reason for hiding this comment

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

Could we easily patch it or would that complicate the build to much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a tad more involved. shairport-sync is built with --without-configfiles meaning the dbus and sample config files aren't installed.

Compiling without --without-configfiles requires a patch to the build system (I have all this in a branch somewhere, just not on this pc otherwise I'd paste it).

Then there's actually two dbus policies:

I've merged them into a single file in this PR.

Patching them is probably a simple s/shairport-sync/${user}/g, but I haven't checked.

Another option is to simply remove the ability to set the user/group, and force it to shairport-sync so the upstream policies can be used directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SuperSandro2000 here's what I mean: vs49688@b0293d2

Copy link
Member

Choose a reason for hiding this comment

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

Do you think you can fix the double prefix issue at its core?

Patching them is probably a simple s/shairport-sync/${user}/g, but I haven't checked.

That would be really simple.

Another option is to simply remove the ability to set the user/group, and force it to shairport-sync so the upstream policies can be used directly.

I am not sure what other people think but if upstream doesn't support it either, that might make things just way easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's this?

vs49688@20bd259

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option is to simply remove the ability to set the user/group, and force it to shairport-sync so the upstream policies can be used directly.

I am not sure what other people think but if upstream doesn't support it either, that might make things just way easier.

I would be 100% for this, just to keep things simple.

Copy link
Member

Choose a reason for hiding this comment

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

How's this?

vs49688@20bd259

Nice. Could you update the PR?

Copy link
Contributor Author

@vs49688 vs49688 Jan 17, 2024

Choose a reason for hiding this comment

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

I can, or we could wait until mikebrady/shairport-sync#1782 is merged and remove the patch altogether.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@FliegendeWurst FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Dec 2, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 2, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
@SigmaSquadron SigmaSquadron removed the awaiting_changes (old Marvin label, do not use) label Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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
Development

Successfully merging this pull request may close these issues.

7 participants