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

Add install files to the build #64

Merged
merged 3 commits into from
Nov 9, 2021

Conversation

matt335672
Copy link
Member

Adds installation files to the build.

make install now also creates the following files:-

  • /usr/libexec/pulseaudio-module-xrdp/load_pa_modules.sh which dynamically loads the xrdp pulseaudio modules
  • /etc/xdg/autostart/pulseaudio-xrdp.desktop which calls to the above for any desktop which supports the XDG autostart specification.

This PR is dependent on #59

This method of loading pulseaudio drivers is compatible with existing systems, and removes the need for PA_SCRIPT to be set in /etc/xrdp/sesman.ini for all systems.

instfiles/Makefile.am Outdated Show resolved Hide resolved
@matt335672
Copy link
Member Author

@metalefty - I'm aware you've approved this, but related to your comment above I've added an extra command line switch to configure to make FreeBSD easier to support.

The following now works for me on FreeBSD 13.0:-

./bootstrap
CFLAGS="-I/usr/local/include" ./configure \
    --with-xdgautostart-dir=/usr/local/etc/xdg/autostart \
    PULSE_DIR=/usr/ports/audio/pulseaudio/work/pulseaudio-13.0
make
sudo make install

If you're happy with this, I'll start merging in the other changes to devel.

@metalefty
Copy link
Member

@matt335672 Thanks, I'm fine with that.

@@ -0,0 +1,32 @@
#!/bin/sh

PACTL=/usr/bin/pactl
Copy link
Member

Choose a reason for hiding this comment

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

Happy if we can avoid this hardcoded path of pactl.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - this will be broken on FreeBSD of course.

I'll assume it's in the PATH. It's not a privileged script, and if the pa utils aren't in the PATH for the desktop, the user will have other problems anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look into this a bit further - my FreeBSD testing was OK, so something's happening here I'm not expecting

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it - under FreeBSD, the PULSE_SCRIPT environment variable is loading the xrdp drivers. The load_pa_modules.sh fails, but that doesn't matter.

Removing the PULSE_SCRIPT definition is causing some problems. I'll get this working on FreeBSD, re-test on a couple of Linuxes and then post an updated script for re-review.

@matt335672
Copy link
Member Author

Script now works OK with my FreeBSD 13 testing, with or without the PULSE_SCRIPT setting.

A further change was required in that on my FreeBSD VM without PULSE_SCRIPT set, the pulseaudio daemon was using the built-in sound card (oss_output.dsp0) rather than the xrdp channel. I've now set the default source and sink to be the xrdp devices if we're in an xrdp session. I can't think of any reason why the user wouldn't want to do this anyway.

I've also retested on Ubuntu 20.04, Ubuntu 18.04, Debian Buster and CentOS 8 with or without the PULSE_SCRIPT setting. They all seem OK, so that leaves the way open to removing that setting in a later version of xrdp.

- Find pactl in PATH
- Set default source/sink for xrdp sessions
- Exit status returned from instfiles/load_pa_modules.sh
@matt335672 matt335672 merged commit 6ac122a into neutrinolabs:devel Nov 9, 2021
@matt335672 matt335672 deleted the instfiles branch November 9, 2021 10:49
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

Successfully merging this pull request may close these issues.

2 participants