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

Support Gentoo Linux #2807

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from
Open

Conversation

mstone2001
Copy link

As requested, breaking up my push into one per platform. This is 1 of 4.

GentooLinux:

Added support for OpenRC and startup files to the build. Added --enable-openrc to install them.
As per comments, replace path in open rc files with actual path from make.

#!/sbin/openrc-run

command="__BASE__/sbin/xrdp"
pidfile="/run/${RC_SVCNAME}.pid"
Copy link
Member

Choose a reason for hiding this comment

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

PID directory might be configured by XRDP_PID_PATH. I don't recommend hardcoding this.

Copy link
Member

Choose a reason for hiding this comment

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

XRDP_PID_PATH appears to map to ${localstatedir}/run in many Makefile.am files, so this could be achieved by expanding the install-data-hook:

Personally I'm not that keen on using install-data-hook in this way - the way the systemd files are done is cleaner to my mind. That is however outside the scope of this change.

Copy link
Member

Choose a reason for hiding this comment

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

Personally I'm not that keen on using install-data-hook in this way - the way the systemd files are done is cleaner to my mind. That is however outside the scope of this change.

I thought the same thing. I didn't mentioned that because that is out of scope so we're totally on the same page.

Copy link
Author

Choose a reason for hiding this comment

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

After reading about these comments and reviewing the code, it seems like there are a couple of different ways the code is currently configuring itself for installs. I was following the install-data-hook pattern. I'd volunteer to open a separate pull request to look at unifying all of the install logic to whatever you think is the best practice way to do it. Since I'm already adding some new platforms with new install files I could look at making them all common. In any case, for the current pull request, do you want me to make the PID_PATH changes?

Copy link
Author

Choose a reason for hiding this comment

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

I resolved the other conversation since we were talking about similar things. I fixed some whitespace errors in the current commit. Let me know what you'd like me to do regarding this pull request, is it acceptable as is or do you want me to make some additional changes?

Copy link
Member

@matt335672 matt335672 left a comment

Choose a reason for hiding this comment

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

No additional comments to @metalefty

configure.ac Show resolved Hide resolved
#!/sbin/openrc-run

command="__BASE__/sbin/xrdp"
pidfile="/run/${RC_SVCNAME}.pid"
Copy link
Member

Choose a reason for hiding this comment

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

XRDP_PID_PATH appears to map to ${localstatedir}/run in many Makefile.am files, so this could be achieved by expanding the install-data-hook:

Personally I'm not that keen on using install-data-hook in this way - the way the systemd files are done is cleaner to my mind. That is however outside the scope of this change.

Replace sbin path in openrc files with path actual path.
Revert "Merge branch 'gentoo' of https://github.com/mstone2001/xrdp into gentoo"
This reverts commit caf7daf, reversing
changes made to 23158bc.
@matt335672
Copy link
Member

Hi @mstone2001,

This PR has suffered slightly in that merging it will cause some regressions in these files:-

  • sesman/chansrc/smartcard.c
  • sesman/chansrc/smartcard_pcsc.c
  • xrdp/xrdp_font.c

Can you rebase to the latest wavefront in devel?

You may not know what I'm talking about - I didn't understand any of this when I made my first PR to this project. The concept is explained pretty well here, but it's a bit of a learning curve if you're used (as I was) to something like Subversion. Please feel free to come back to me with any questions.

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.

3 participants