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

Inclusion in Debian ? #3

Closed
MoonSweep opened this issue Dec 17, 2023 · 26 comments
Closed

Inclusion in Debian ? #3

MoonSweep opened this issue Dec 17, 2023 · 26 comments

Comments

@MoonSweep
Copy link

MoonSweep commented Dec 17, 2023

Hi,

I used to use the old pulseaudio-module-xrdp and it was cumbersome to compile, needing to run various scripts to extract the required headers and run some scripts, etc.

Now this new pipewire-module-xrdp has a much more streamlined compilation process, needing only some build dependencies and no extra steps from the common Debian package building tools (./bootstrap is not even required, as dpkg-buildpackage runs autoreconf by itself before the usual ./configure, make and make install). Moreover, more and more desktops move away from pulseaudio to pipewire.

So, my question is, do you plan to include this module into Debian ? Or even better, include it in the main xrdp source code ? This would be even better, since package maintainers could then create xrdp packages with audio redirection working out-of-the-box.

If you plan to ship it separated from xrdp main source code, I can provide the debian/ directory I wrote (all you'll have to do is change my e-mail address and maybe check and fix the copyright file).

Regards,

--
Raphaël

@MoonSweep
Copy link
Author

Ping ?

@elboulangero
Copy link
Contributor

Already done:

What needs to happen next:

  • @matt335672 Do you think it's a good idea to tag a release? That would be a good signal for distros that the repo is ready to be packaged.
  • if there's a tag, then I'd follow up with the Utopia team in Debian, so that the maintainers of the pipewire package in Debian can review my package and fix what needs be

Alternatively: there was some discussions to include this module straight in pipewire, cf. neutrinolabs/xrdp#2023 (comment). Pipewire upstream was willing to accept the module, however the author @Hiero32 never followed up, as far as I know.

Is there still a plan to include this module upstream, is anyone aware of anything?

@matt335672
Copy link
Member

They're all good points @elboulangero.

Arguments for upstreaming

  1. A lot of the maintenance of the module will now be done by the pipewire team. For example, a lot has changed in the module-pipe-tunnel.c module since @Hiero32 split this off. We should probably adopt some of these changes here.
  2. Supporting multiple pipewire versions might be messy if the module isn't delivered with Pipewire.

Arguments against upstreaming

  1. The pipewire team are unlikely to test any of this module unless we can also provide some kind of test harness. This will require maintenance, so we may not save a lot of time against 1) above anyway.
  2. Any changes will require us to update two projects now instead of one.
  3. Unlike Pulseaudio, Pipewire supports out-of-tree modules anyway, so building this is a walk in the park compared to the previous situation.

Personally I'm leaning towards not upstreaming, but that's just my view. @metalefty, @jsorg71, @Nexarian, @Hiero32 - do you have views on this? Please add to either section in the list above.

@elboulangero - regardless of the outcome of the above discussion, would you like a release of the current module? If so, I'll look at addressing some of cosmetic issues. We have at least one spelling mistake (conect_xrdp_socket()), and a clash of spaces and tabs. For the record, Pipewire sources use tabs for tabbing and xrdp sources use spaces. This module currently uses both! We should probably use tabs here.

@MoonSweep
Copy link
Author

Hi,

I'm not part of the project so this is only my humble opinion, but I would find it more logical if it was included in XRDP distribution, rather than PipeWire.

This way, maintainers of XRDP packages in distributions would only need to add PipeWire headers as build depends, and the module could be built automatically. Then, it can be shipped in a separate binary package depending on PipeWire, as not to make the whole XRDP package depend on PipeWire.

On the other hand, maintenance from the PipeWire team would of course be greatly useful, but as said above, it would be difficult for them to test it, as it would need a ready to use XRDP server, which is completely out of scope for their project.

@elboulangero
Copy link
Contributor

@matt335672 Thank you for summarizing the pros and cons of upstreaming vs separate module.

@elboulangero - regardless of the outcome of the above discussion, would you like a release of the current module?

Yep, if that's Ok with you, I'd be happy with that. Then I can kickstart an attempt to get this into Debian proper, as I have a bit of bandwidth these days.

@MoonSweep Apparently, in terms of build depends, there are not dependency on xrdp, so it's not shocking it it becomes part of pipewire. It will not make pipewire depend on xrdp at build time. And I suppose that, even if it's part of pipewire, it will still be under the form of a module, so it can be shipped as a separate package if needed.

Cheers

@matt335672
Copy link
Member

Well, this version works and people are using it. I see no reason to mess about with getting the spacing consistent at this stage, or to fix spellings. We can do that for the next release, or for getting the module into pipewire itself.

@metalefty - would you like to make a release so this is signed with your key? I'm happy to make one, but using the same signing key as xrdp seems to make more sense to me.

@metalefty
Copy link
Member

I would rather like to keep maintaining by us because it is easier to build out-of-tree modules compared to PulseAudio as you mentioned, however, I personally happy to push this to upstream if PipeWire team is willing to maintain.

I will make a release soon with sign.

@metalefty
Copy link
Member

I made a release. The tarball is not added to the release because it does not include submodules, unlike xrdp main source. pulseaudio-module-xrdp also does not have a tarball other than generated by GitHub. Let me know if it is needed.

https://github.com/neutrinolabs/pipewire-module-xrdp/releases/tag/v0.1

@matt335672
Copy link
Member

That looks fine to me - thanks.

@elboulangero - is that enough for you to be getting on with?

@elboulangero
Copy link
Contributor

elboulangero commented Mar 7, 2024

All I need is a tag, and I see a tag v0.1. Enough for me! I'll keep you updated. Thanks!

@elboulangero
Copy link
Contributor

@MoonSweep
Copy link
Author

MoonSweep commented Mar 16, 2024

Hi, sorry to insist, but...

In #1037111, your update states that "this module will not be included in pipewire upstream, and it will be maintained by neutrinolabs (which is the upstream for xrdp already)".

In that case, wouldn't it be much simpler to include the module in the xrdp source tree, instead of creating a new package ?

Are the maintainers of xrdp against this and want to keep the two projects as separate upstreams ?

Regards,

--
Raphaël

@matt335672
Copy link
Member

Hi @MoonSweep

I think 'simpler' may depend on your perspective. From ours, two separate projects are simpler, particularly as this module is effectively an interface between pipewire audio and xrdp. We only control one side of that arrangement.

I note that in the past, Debian have packaged two tarballs from us in the same Debian package - that might be an option here if you want to make things easier for the users:-

https://snapshot.debian.org/package/xrdp/0.9.1-1/

Thoughts?

@MoonSweep
Copy link
Author

MoonSweep commented Mar 16, 2024

Hi @MoonSweep

Hi @matt335672

I think 'simpler' may depend on your perspective. From ours, two separate projects are simpler, particularly as this module is effectively an interface between pipewire audio and xrdp. We only control one side of that arrangement.

I don't quite understand why it's simpler, but I won't insist on that point. It's just that, even if it doesn't need xrdp sources to compile, it would be useless without it, so I see it more like an xrdp module than a pipewire module. Hence my logic, including it with xrdp seems more natural to me.

I note that in the past, Debian have packaged two tarballs from us in the same Debian package - that might be an option here if you want to make things easier for the users:-
Thoughts?

Not simpler for the users, but for the maintainers (both are maintained by a team, Debian Remote) . Those two tarballs are the xrdp server itself, and the "virtual" drivers to start an X server from it (because IIUC it can also be used as a relay to a vnc or another rdp server).

Adding a new tarball wouldn't be easier for them, on the contrary, since they would need - I guess - to add new steps in debian/rules to compile the module along the rest.

But if this work is already done upstream, and xrdp's Makefile already compiles the pipewire module by itself if it detects that pipewire's headers are installed, the additional work would just be to add a new build-dependency and the new binary package (i.e. only modify debian/control and add one .install file, and only once, when the new upstream version including the pipewire module is released).

You could even do that for them if you think that it wouldn't be fair to add work for them without asking (I admit I didn't think about this until now), or even join the team if you like (since you're already a maintainer IIUC), it would still be much less work for you than creating a new package, having it included in Debian, and maintaining it on the long term.

Sorry if I'm not clear, English is not my native language ^^'

@matt335672
Copy link
Member

I think you're plenty clear enough @MoonSweep - it's probably me that's not being clear. My apologies.

The xorgxrdp package contains the Xorg drivers that allow us to use Xorg directly from an RDP session. It's quite closely coupled with xrdp and is useless without it.

For xrdp-0.9.1, Debian decided to package the two tarballs from us as a single Debian package. The reason was, that, for this version Debian decided to make the xorgxrdp backend the default rather than the older VNC backend.

For 0.9.6-1, Debian decided to make these two packages separate (Changelogs here and here). That's a decision that Debian Remote took without consulting us. We're absolutely fine with that. You would expect Debian Remote to not only grasp the finer details around Debian packaging, but to understand the needs of their user community better than upstream (i.e. xrdp, and other projects).

Other projects provide a single tarball which is split into several Debian packages. This doesn't apply to xrdp, but does (for example) to pulseaudio.

In other words, there's no real tie between the number of tarballs we deliver, and how the various distributions (not only Debian) present that software to their users. That's a decision for them, and not for us.

I don't think our decision regarding our own project deliverables makes it any easier or harder for the distro packagers. They're competent engineers, and providing we stick to standard packaging and configuration tools they can cope easily with anything we throw over the wall to them.

Other distros have made use of xrdp and xorgxrdp being separate deliverables. Arch currently has two packages, xorgxrdp and xorgxrdp-glamor where xorgxrdp is built with different switches. Personally I'd question the wisdom of that, but I'm not an Arch packager!

Given the above background, at the moment it seemed sensible to me to deliver the pipewire audio driver in the same way that xorgxrdp is delivered. The two are both 'interface packages'. They are similar in that:-

  • Neither is useful on its own
  • If the interface on the other side changes (i.e. Xorg or pipewire), it's a lot easier for us to change the interface package to support the new interface, or even to create a new interface package.

@MoonSweep
Copy link
Author

I understand your point. I won't make any more comments on this.

@elboulangero
Copy link
Contributor

elboulangero commented Mar 26, 2024

Hello, I opened 3 MR so far.

!7 and #10 are mostly about tidying up.

!8 is really needed for inclusion in Debian though, thanks to this change we can safely install the module on a system where pipewire is not installed, and in this case it won't do anything (the xdg autostart script will just exit early).

@metalefty If it's all good for you, I'd be happy if you could merge those 3 PRs, tag a new version, and then I'll be good to proceed with Debian packaging, and upload the package in Debian. Thanks thanks thanks!

@elboulangero
Copy link
Contributor

I don't think our decision regarding our own project deliverables makes it any easier or harder for the distro packagers. They're competent engineers, and providing we stick to standard packaging and configuration tools they can cope easily with anything we throw over the wall to them.

Fully agree with @metalefty here! Upstream knows best how to handle their stuff, and whether it's better to split in different Git reps, or ship it all in a single Git repo. Downstreams (eg. Linux distros in this case) will adjust accordingly, and will then know best how it should be distributed (eg. one package, or split into several packages) so that it fits in the distro as a whole.

In this case I'm pretty happy that it's a separate Git repo. It's all about pipewire, therefore it will be maintained within the Debian team that already maintains the pipewire and wireplumber packages. If ever there are changes in the pipewire lib that requires a rebuild and/or adjustments in the module, they will be able to help with it, without having to deal with the whole xrdp repo. I think it's just perfect as it is :)

@metalefty
Copy link
Member

Hi @elboulangero,

I also do package maintenance in FreeBSD project so I think understand the manners of both sides. It is very grateful to submit useful changes from downstream to upstream. Unfortunately, PipeWire is not yet my area, so I won't be able to review your changes from corner to corner. However, your changes look nothing weird, we're happy to accept them.

Your contribution is definitely helpful, thanks!

@metalefty
Copy link
Member

Merged all PRs and created a new tag.

@elboulangero
Copy link
Contributor

Fully agree with @metalefty here!

Actually I quoted a part from @matt335672 in my comment above, sorry for the confusion.

@metalefty Thanks for merging! I updated the Debian packaging, and now waiting for one last round of feedback from Dylan, before uploading the package to Debian. Almost there!

@matt335672
Copy link
Member

Fine by me @elboulangero!

Thanks for the effort you're putting into this. Having a pre-built module to do this work will help enormously with user issues.

@elboulangero
Copy link
Contributor

The package pipewire-module-xrdp is now in Debian's NEW queue: https://ftp-master.debian.org/new.html

It means that it's uploaded, and waiting from review from the FTP masters, before it's accepted in Debian unstable. This step can take an unpredictable amount of time. I'll ping again here when the package is accepted.

@MoonSweep
Copy link
Author

The package pipewire-module-xrdp is now in Debian's NEW queue: https://ftp-master.debian.org/new.html

Great news ! Thanks for all your efforts :)

@elboulangero
Copy link
Contributor

And now in Debian unstable: https://tracker.debian.org/pkg/pipewire-module-xrdp

We can close this issue, Thanks everyone for following up until the end!

@matt335672
Copy link
Member

I think the thanks are mostly due to you @elboulangero !

I'll close this now then.

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

No branches or pull requests

4 participants