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

media-video/{pipewire,wireplumber}: add system-service USE flag #23972

Closed
wants to merge 1 commit into from

Conversation

zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Jan 26, 2022

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @zx2c4
Areas affected: ebuilds
Packages affected: acct-group/pipewire, acct-user/pipewire, media-video/pipewire, media-video/wireplumber

acct-group/pipewire: @gentoo/proxy-maint (new package)
acct-user/pipewire: @gentoo/proxy-maint (new package)
media-video/pipewire: @gentoo/gnome, @thesamesam, @a17r, @Whissi
media-video/wireplumber: @pinkflames, @thesamesam

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.

If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added new package The PR is adding a new package. assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Jan 26, 2022
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2022-01-26 21:50 UTC
Newest commit scanned: ac56e57
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/458462d7fd/output.html

@pinkflames
Copy link
Contributor

pinkflames commented Jan 26, 2022

A few things:

  1. Upstream may or may not publish proper guidance on how to use system wide mode while also documenting its drawbacks, so I'd recommend waiting for that to happen.
  2. It's probably less secure and more complicated to deploy than using a regular user set up to autologin after bootup, so it would be best to offer instructions for that as an alternative to system-wide.
  3. I would like to see the USE flag masked just like system-wide is masked for media-sound/pulseaudio.
  4. Optional but definitely relevant is that RTKit won't work, so the pipewire user will need proper RLIMITs based realtime to avoid xruns (or it could be instead run as root starting with the not yet released 0.3.44).
  5. Has this been tested to actually work? I would expect that merely enabling the units would actually lead to no audio for everyone as the most likely outcome, unless there's some additional steps taken.
  6. I would need to check the build system but I expect it to be a pure static file change of a small additional file being installed, so according to the Gentoo QA rules it should really be enabled unconditionally (or along with systemd IUSE), which will of course invite problems. But, I guess, one could always ask for a QA exception, especially if the USE flag in question was package.use.masked.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Jan 26, 2022

so I'd recommend waiting for that to happen.

They've already mentioned in various comments that system mode is used by people and works fine. There's nothing to wait for here. The feature is there. It works. It's ready.

so it would be best to offer instructions for that as an alternative to system-wide.

I can add mention of that to the ewarn if you want, no problem.

I would like to see the USE flag masked just like system-wide is masked for media-sound/pulseaudio.

Okay. I can do that.

Optional but definitely relevant is that RTKit won't work

Right, this is relevant. I can include that in the ewarn too.

Has this been tested to actually work?

Yes, and it works incredibly well. Basically no complaints. Once this lands, I can augment the Gentoo wiki with my experience and configuration if you want.

I would need to check the build system but I expect it to be a pure change of a small install file, so according to the Gentoo QA rules it should really be enabled unconditionally (or along with systemd IUSE), which will of course invite problems. But, I guess, one could always ask for a QA exception, especially if the USE flag in question was package.use.masked.

It also pulls in the acct-{user,group}, so it's not just installation of two files that should be done unconditionally.

@pinkflames
Copy link
Contributor

pinkflames commented Jan 26, 2022

I'm not sure if there's many people using it. Most real users are proper embedded deployments like AGL (Automotive Grade Linux). What I'm referring to is upstream specifically documenting the know-how related to doing such deployments properly. The reason Gentoo has arguably good PIpeWire experience is that we have the know-how for desktop deployments, and, I think, we should also have the know-how for embedded deployments, before we offer that (assuming Gentoo supports embedded at all, which is not what I heard from Gentoo developers when this last came up).

No, do not warn that RTKit won't work (people won't know or care, etc). Instead explain what they need to do to fix their possible xrun issues (or do it for them if you can i.e. include a PipeWire specific and correct limits.d file that gives pipewire user the required realtime capabilities; but in that case only change the 9999 ebuilds for now since no released version currently has the unified module-rt which uses RLIMITs first and then falls back to RTKit).

From what I know I would not expect this to work, because software will still be looking for /run/user//pulse/native and /run/user//pipewire-0 sockets rather than the ones provided by the system daemon. Speaking of which, does this even work with, say, Wayland and OBS? Something tells me this has never been attempted/tested with the system daemons and therefore should be also documented, if it does not work.

Extra stuff being documented at Wiki or upstream is fine. But, if something needs to be always done to work at all, please, have the elog/ewarn print those instructions as briefly and as uniformly applicably as possibly (i.e. nothing specific to particular shell or window manager).

From what I can tell, acct-{user,group}/ is still not changing the build configuration, so it still falls under the same rule that it would, IMO, need a QA exception but perhaps I'm wrong.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Jan 26, 2022

think, we should also have the know-how for embedded deployments, before we offer that

Note that I have in mind server usage rather than embedded usage. Anyway, I'm fine masking the USE flag until whatever point in the future you're comfortable with.

From what I know I would not expect this to work, because software will still be looking for /run/user//pulse/native and /run/user//pipewire-0 sockets rather than the ones provided by the system daemon.

Streaming to a tcp server actually works quite well.

@pinkflames
Copy link
Contributor

pinkflames commented Jan 26, 2022

To me this means it really should be behind package.use.mask explaining that it's experimental, unsupported and whatever thing people might be doing may not have been verified as working at all.

Update: Since my original reasoning behind re-using the realtime group was to avoid creating a dedicated pipewire group, we could just install the upstream example and RDEPEND on acct-group/pipewire unconditionally.

And then you need to worry about QA and what might other people think/say/want.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Jan 27, 2022

I'll rebase this on the new version of pipewire that was just released when the ebuild is bumped, and I'll try to synthesize your input into something I think makes sense. I think I have an idea of what you're getting at, so I'll do my best and you can comment on the updated PR when it's ready. I had one question though:

we could just install the upstream example

I like this idea of installing the rlimit file, but I was wondering: is it better to do this as part of acct-user/pipewire or media-video/pipewire? I was thinking it'd be better to do this as part of acct-user, since wireplumber uses that same user in system mode. But maybe there's a policy concern about installing files with user accounts?

@pinkflames
Copy link
Contributor

pinkflames commented Jan 27, 2022

I'm a proxy maintainer (of WirePlumber), and I'm not really well versed in the fine details of Gentoo packaging rules but, yes, I would not have acct-{user,group} install files, if it can be helped.

Luckily media-video/pipewire is the actual user of RLIMITs. Specifically /usr/lib64/pipewire-0.3/libpipewire-module-rt.so which is part of PipeWire. The way WirePlumber uses RLIMITs is to also load that module for its own uses. Therefore placing the limits.d file for @pipewire inside media-video/pipewire is very sensible.

And in the impossible situation that WirePlumber was installed without PipeWire being present, making pipewire group lack the appropriate RLIMITs, it's not a catastrophic failure. Just an increased chance of Bluetooth issues (this is an imaginary scenario since wireplumber.service will probably not even try to start due to missing pipewire.service provider).

@zx2c4
Copy link
Contributor Author

zx2c4 commented Feb 4, 2022

I've rebased this on the latest ebuild and incorporated a few suggestions and made some decisions:

  • I now include the recommended limits.d snippet to the limits.d file that we're installing for USE=system-service.
  • I'm keeping the USE flag, as having warnings about this and limiting user confusion seems like a good idea. Plus, the expanded set of users and the potentially more porous limits.d file means maybe we don't want it by default.
  • I didn't mask the USE flag, as this is an upstream supported configuration and does work very fine without hacks.

@thesamesam please pull!

@thesamesam
Copy link
Member

please fix CI issue, I'd like to still mask it for now to discourage use but we can revisit (If unmasked I'd want to make sure we had docs on wiki and maybe in pkg_postinst)

Sorry, on mobile due to HW issues but will pull in this evening! Thank you!

@zx2c4
Copy link
Contributor Author

zx2c4 commented Feb 4, 2022

Oh wow 506 got taken in the intervening time. Fixing QA issues, will add USE mask, and will resubmit.

Package-Manager: Portage-3.0.30, Repoman-3.0.3
Signed-off-by: Jason A. Donenfeld <zx2c4@gentoo.org>
@zx2c4
Copy link
Contributor Author

zx2c4 commented Feb 4, 2022

Alright, all set. Fingers crossed that CI is green this time.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2022-02-04 20:36 UTC
Newest commit scanned: bc7f0bd
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/a4b8a8584f/output.html

@pinkflames
Copy link
Contributor

As long as it's package.use.masked I'm okay with this being merged, so ACK from me. :)

Regarding upstream support state, let's say it's not not-supported. However upstream themselves are only using this with Automotive Grade Linux, which generally involves exceptionally old software by desktoop Linux standards in a true embedded deployment. And I would be surprised if there's any system service specific CI being done to ward against breaking changes. On top of that it's also almost(?) entirely undocumented, requires manual setup and while it's probably safe for networking a headless server, it may pose a security issue if someone decided to have multiple local users sharing the same system daemon. All of those are good reasons why it should not be a USE flag that users can toggle without having at least had a proper warning in the form of needing to unmask the flag.

@thesamesam
Copy link
Member

Not one commit per package? :(

@gentoo-bot gentoo-bot closed this in 8d6a825 Feb 5, 2022
gentoo-bot pushed a commit that referenced this pull request Feb 5, 2022
Closes: #23972
Package-Manager: Portage-3.0.30, Repoman-3.0.3
Signed-off-by: Jason A. Donenfeld <zx2c4@gentoo.org>
gentoo-bot pushed a commit that referenced this pull request Feb 5, 2022
Closes: #23972
Package-Manager: Portage-3.0.30, Repoman-3.0.3
Signed-off-by: Jason A. Donenfeld <zx2c4@gentoo.org>
gentoo-bot pushed a commit that referenced this pull request Feb 5, 2022
Closes: #23972
Package-Manager: Portage-3.0.30, Repoman-3.0.3
Signed-off-by: Jason A. Donenfeld <zx2c4@gentoo.org>
@zx2c4 zx2c4 deleted the system-service-pipewire branch February 5, 2022 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). new package The PR is adding a new package. no bug found No Bug/Closes found in the commits.
Projects
None yet
5 participants