evertiro / cdm

Console Display Manager
http://evertiro.com
GNU General Public License v2.0
428 stars 40 forks source link

Small change to call to startx, run in subshell. #21

Closed rolfmorel closed 11 years ago

rolfmorel commented 11 years ago

cdm didn't do/start anything without this change on my system, Ubuntu 12.10, $BASH_VERSION=4.2.37(1)-release.

After being able to trace it back to setsid startx ... line, the problem seemed to be the fork of setsid. If I didn't fork (no &), it worked but the script waited for return of setsid/startx.

This patch backgrounds the subshell in which setsid runs, which works for me but it seems it is quite finicky about the setsid command.

It might be helpful if there was some discussion about what works on different systems (and maybe why it works/doesn't work).

rolfmorel commented 11 years ago

Even though this was needed to get it to work on that ubuntu system, it is nasty in job management terms in that by backgounding the subshell, that subshell still waits until setsid returns. This means that it keeps around processes that are unnecesary, and makes this nothing more than a nasty hack for that (particular) system.

Now testing this on archlinux and finding that both solutions, with this patch and without, work I would rather revert this patch and when anybody else has a issue on their system review a proper solution.

So in short, as author of this hack, I move to revert it as it is a hack which does not belong in upstream.

evertiro commented 11 years ago

I suggest making this a configuration option. While I agree that hacks don't belong in upstream, if the hack is necessary in some situations, it might be easier for us to make it an established configuration option than answer the support questions we'll inevitably run into down the road. Thoughts?

rolfmorel commented 11 years ago

I agree, it is probably best to make it a configuration option. And your right about the support questions and providing this as an easy fix for users having (this particular) problem(s).

I will write a simple patch that adds an option to cdmrc, altstartx seems some what descriptive. That should keep upstream relatively clean.