Merge lp://qastaging/~raof/mir/xserver-spawner into lp://qastaging/mir

FAILED: Continuous integration, rev:1349
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~raof/mir/xserver-spawner/+merge/203475/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/738/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/752/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/749/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/353/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/468/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/472/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/354/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/738/rebuild

How much should Mir know about launching xservers?

Is it essential to all the uses we envisage: unity8? system compositor? Other (hypothetical at this time) shells?

could
+const char* mir::X::NullServerContext::client_connection_string()
return a string instead of a const char*?

> How much should Mir know about launching xservers?
>
> Is it essential to all the uses we envisage: unity8? system compositor? Other
> (hypothetical at this time) shells?

Last I heard from the architectural-level view is that lightdm was going to negotiate starting mir and friends and hooking up the FD's between them appropriately. That was something I heard a (relatively) long time ago though.

@Alan:
I think most non-trivial shells are going to want to support running X11 apps. It's not interesting for system-compositor usage, true.

As to why Mir should know about launching xservers - the process of tying together foreign X11 windows needs Mir support. Mir will need to include an X11 window manager in order to proxy between X11 EWMH window management and Mir - this could be in U8, but any shell that wants to support X11 apps will need identical code.

Ideally you want to start the X11 WM before any app connects to the server to avoid unexpected behaviours; the easiest way to do this is to start the X server, wait for it to come up, connect the WM proxy, and only then consider the server to be started (that's the next part of X11 integration).

I (obviously) think it makes sense to have Mir know how to start the X server.
@kdub:
This is somewhat of a circular dependency - the X server needs Mir running in order to be started (it needs the mir socket), and Mir's X11 WM needs the X server to be running. You can't easily start both from LightDM, as it would need to start Mir, it to be ready to accept connections, then start the X server.

Also, U8 wants to (a) start an X server only when an app needs it, and (b) start one-Xserver-per-app.

re: client_connection_string - Yeah, it could return a std::string (and did, at one point). I switched to char const* because I only ever wanted to consume it from things that took a char const*, but can easily switch it back.

(1) "er" issues as described above.

(2) I'm concerned about mentioning "X"-anything in the Mir source too. It feels like we're preventing Mir from being a success in its own right if it even has to mention X in its source. Even if the coupling is weak, we should strive for a better answer. Generalize, move it out-of-project, etc. We almost certainly don't want an "X" namespace. That's a reasonable indication that we're going in the wrong direction.

(3) You've introduced a build-dep on libx11-dev:
  +#include <X11/Xlib.h>
Definitely don't do that, at least :)

On Wed, 2014-01-29 at 03:09 +0000, Daniel van Vugt wrote:
> Review: Needs Fixing
>
> (1) "er" issues as described above.

Would you prefer XServerFactory?

> (2) I'm concerned about mentioning "X"-anything in the Mir source too. It feels like we're preventing Mir from being a success in its own right if it even has to mention X in its source. Even if the coupling is weak, we should strive for a better answer. Generalize, move it out-of-project, etc. We almost certainly don't want an "X" namespace. That's a reasonable indication that we're going in the wrong direction.

We're going to have to support running X11 apps for the foreseeable
future. Certainly for the life of 14.04, probably for the life of 16.04.

I don't think it's unreasonable for the Mir codebase to reflect the
importance of legacy X11 support.

Indeed, the actual implementations should be in a separate opt-in
libmirserverX11integration.so library, and will be once they start to
depend on X11 libraries.

>
> (3) You've introduced a build-dep on libx11-dev:
> +#include <X11/Xlib.h>
> Definitely don't do that, at least :)
>

Ah, yeah. This is currently only for the (disabled) acceptance test.
Once there's mirserver code that depends on X11 libs it'll be optional
and in a separate library.

@er:
I still dont get the argument about "er" - If RAOF renames "XServerSpawner" into "XServerFactory" and people are satisfied then there is something wrong with that rule (of thumb?).
We then switched from a noun that was derived from a verb that meant something like: "creates and launches a process or activity" to something that only means: "creates something". So from a clearer specific term to a generic one. A specific term that describes what should be in this class and what not - A term that suggest that this is actually a very specific functor that launches an XServer.

So I would prefer XServerSpawner::create_server over XServerFactory::create_server. Something like is XServerFactory::spawn_server also a good name - I just dont get the reasoning with the "no-er"-rule.

@library: I do believe that this functionality is something a current linux compositor framework needs to provide, but probably as a separate library. -> Needs Fixing

@clang-format: thanks for fixing!