Porting unity-system-compositor to MirAL

[jsalatas]

unity-system-compositor links directly to libmirserver will make it harder for Yunit to incorporate Mir releases as they happen as this requires rebuilding against a new libmirserver ABI.

This was suggested by Alan Griffiths at https://forum.yunit.io/viewtopic.php?f=6&p=285

[WebDrake]

@AlanGriffiths I'm curious about this, though I'd really appreciate a bit more info before I commit to actually doing this (time is limited...). I've never worked on this kind of project before (which is part of why it's interesting) so would appreciate any advice you can share on getting started and getting up to speed.

First question: how come there is a dependency on libmirserver in the first place? I would have assumed that even for something as low-level as this, all the mir dependencies would be on the client API.

Second, can the immediate focus here be on stripping out any references to mirserver, or will it also be necessary to also replace the mirclient stuff with the MirAL API? Or can these be considered two separate concerns?

Third, it seems fairly trivial to build the present code on an Ubuntu 16.04 system once the dependencies are installed (all from repos), but is there any particular setup I should aim for (particularly w.r.t. getting set up with having MirAL available, since I don't see any libmiral in 16.04 repos)?

Finally, are there any particular libraries or technologies that I should make sure to read up on and understand properly before diving into this?

Thanks in advance for any advice that you can share.

[jsalatas]

@WebDrake libmiral is not in the standard repos of 16.04. It is however in yunit's repo for 16.04.

[AlanGriffiths]

@WebDrake I'm happy share advice. We all have to make good use of limited time.

Q1: Mir makes a strong distinction between clients and servers. Clients can paint surfaces and place them within a window, but the server controls the display and organizes the windows. Unlike X11 (for example) the shell logic exists in the server process which requires libmirserver.

Q2: you've understood correctly, it is only the direct dependencies on libmirserver that are at issue here. (See the references below for the reasons.)

Q3: MirAL wasn't an official project when Ubuntu 16.04 was released and adding a new project to an LTS needs more justification than currently exists. MirAL is in the 17.04 archive, but I suspect you will need some small additions to this code, so building it yourself will likely be necessary in any case.

For development purposes you can follow the build instructions in the project directory, followed by:

$ sudo make install && sudo ldconfig 

This will install into /usr/local/ which avoids disrupting your system files. You can clear this out with:

$ find /usr/local/ -type f,l | xargs sudo rm

I'd recommend this over using the yunit ppa on a development system (at the current time I think that is only ready for "disposable" test systems).

A few of references for background:
https://insights.ubuntu.com/2017/04/03/the-miral-story/
https://www.slideshare.net/AlanGriffiths25/the-miral-story
http://voices.canonical.com/alan.griffiths/

[WebDrake]

@AlanGriffiths thanks very much for the detailed reply and the references!

I've cloned upstream MirAL and unity-system-compositor using git-remote-bzr (since these seem to include more recent changes than the repos here) and succeeded in building and running the tests for both of them, and I've verified that my /usr/local MirAL gets picked up OK if I add an extra rule to USC's cmake scripts.

I'll take a while to read and understand the references (and code) as best I can, and follow up once I have results and/or further questions.

[WebDrake]

One quick further question. Do I understand right that the mirserver dependencies essentially amount to every include of a mir/something header other than mir_toolkit and mir/events ... ?

[AlanGriffiths]

Not quite. The ones from mircore are also OK...

$ find /usr/include/mircore/mir /usr/include/mirclient/mir/ -type f
/usr/include/mircore/mir/optional_value.h
/usr/include/mircore/mir/int_wrapper.h
/usr/include/mircore/mir/fatal.h
/usr/include/mircore/mir/geometry/forward.h
/usr/include/mircore/mir/geometry/length.h
/usr/include/mircore/mir/geometry/point.h
/usr/include/mircore/mir/geometry/rectangles.h
/usr/include/mircore/mir/geometry/dimensions.h
/usr/include/mircore/mir/geometry/size.h
/usr/include/mircore/mir/geometry/displacement.h
/usr/include/mircore/mir/geometry/rectangle.h
/usr/include/mirclient/mir/events/event_builders.h
/usr/include/mirclient/mir/event_printer.h

[WebDrake]

What about mircommon and mirplatform?

[AlanGriffiths]

I've not investigated, but I suspect any of those in USC those will be transitive dependencies through mirserver.

[WebDrake]

OK. I'm going through the codebase right now and will post a summary of findings/speculations about what needs to be addressed and how.

[WebDrake]

Brief summary of where I'm at.

There are 2 ways in which mir functionality is included in USC: one via direct import of mir headers (#include <mir/....>), the other by direct declaration of mir functionality inside USC's own headers (e.g. namespace mir { namespace frontend { class Session; }};).

So, we can search for files that have these direct dependencies as follows:

$ find ./src/ -type f -print0 | xargs -0 grep -e "include <mir" -e "namespace mir" | cut -d : -f1 | uniq | sort
./src/clock.h
./src/dbus_event_loop.h
./src/display_configuration_policy.cpp
./src/display_configuration_policy.h
./src/main.cpp
./src/mir_input_configuration.h
./src/mir_screen.cpp
./src/mir_screen.h
./src/screen_event_handler.cpp
./src/screen_event_handler.h
./src/screen.h
./src/server.cpp
./src/server.h
./src/session_monitor.h
./src/session_switcher.cpp
./src/system_compositor.cpp
./src/system_compositor.h
./src/window_manager.h

Note, this is incomplete given that .cpp files that include the header files listed here will also likely have hard dependencies on mir functionality.

Let's examine a few of these in detail:

• display_configuration_policy.* has a direct counterpart in MirAL's display_configuration_option.* and can be replaced by it. Strictly speaking it's not a drop-in replacement (the MirAL code adds alpha translucency settings) but we can treat it as such in the short term. Just doing this alone buys us little, however, since it just tweaks one line of the much more problematic usc::Server class. In the long run we would instead provide this as an input to MirRunner::run_with (see below).

• main.cpp carries an indirect dependency via usc::Server (more on this below) and a direct dependency via its catch(...) block, which uses mirserver's error reporting. This needs to be replaced by the use of MirRunner, and it will basically come down to something like:

int main(argc, char const* argv[])
{
    MirRunner runner{argc, argv};
    // ... set up some other required data structures ...
    return runner.run_with(required, data, structures);
}

... since MirRunner::run_with takes care of error handling for us. The tricky bit is those required, data, structures (which would include display_configuration_options), where we need to unpick various things that are all conflated via usc::Server and usc::SystemCompositor.

• system_compositor.* defines the usc::SystemCompositor class, which largely a wrapper around the call to usc::Server::run. It also contains various private shared_ptr instances, which are all AFAICS assigned from methods of usc::Server, either on construction (in the case of the spinner field, assigned from server->the_spinner) or in the server's init callback (defined in the SystemCompositor::run method).

  • it's really not obvious to me why SystemCompositor has any of these fields, since as far as I can see it does not own them and does not use them outside of the callback.

• server.* defines the usc::Server class, which is a private subclass of mir::Server, and hence is at the heart of the lack of abstraction. It uses various methods of the mir::Server class to provide public access to needed data structures, and also defines various public methods of its own to provide shared_ptr access to data structures that it owns.

  • these latter data structures are all owned via private mir::CachedPtr fields. AFAICS CachedPtr is a wrapper around weak_ptr that looks like its main purpose is to ensure that the order of construction of different required data structures will be correct; that is:

    • for every CachedPtr field blah one defines a wrapper getter method (the_blah) whose job is to define the callback passed to the CachedPtr in order to instantiate the underlying data

    • this callback is only used if the underlying weak_ptr is not already instantiated

    • the callback only ever accesses other data structures via their corresponding the_something getter method, so ensuring that the underlying data will be lazily instantiated as required (propagating backwards through all the dependencies)

• usc::Server also does a lot of overriding of default mir::Server settings via its constructor.

Action items

Based on the above, as far as I can see the work required can broadly be split into two parts:

• unpick the conflation of local and mirserver-based data structures in usc::Server into data structures that can be provided via the MirRunner::run_with method (in other words, to callbacks, functions, or classes defining the call operator, that take a mir::Server& parameter as input).

• upstream to MirAL any of that functionality that directly depends on the mir::Server API.

  • If I understand right, it's OK for ABI compatibility for USC to make use of functionality that takes mir::Server& as input (IIUC ABI-wise this is just a pointer), but it's not acceptable for it to define any functionality that uses mir::Server directly. This is why for example it's OK to invoke MirRunner::run_with using the display_configuration_options function defined in miral/display_configuration_options.h, but not to use the add_display_configuration_options_to defined in USC's own display_configuration_policy.cpp.

Does all of this make sense? Have I misunderstood anything, or does this sound like a viable way forward?

[WebDrake]

it's really not obvious to me why SystemCompositor has any of these [shared_ptr] fields, since as far as I can see it does not own them and does not use them outside of the callback.

Ah, re-reading weak_ptr documentation I think I see why: is it to ensure that, once created, the underlying data structure will never be destroyed for as long as SystemCompositor exists?

[AlanGriffiths]

That sounds like a very good summary of the current status. Your plan is very like what I did with the QtMir code, so there's reason to think it viable.

As for the use of CachePtr - I'm not entirely convinced this is appropriate here. I suspect it exists in USC for historical reasons and is just a copy of an idiom used internally by Mir. Vis:

This allows single-instance parts of the system infrastructure to be lazily-constructed, so long as something in the system is holding a shared_ptr<> to the part the weak_ptr<> will refer to it. But as soon as the last shared_ptr<> goes the part will be destroyed (and the weak_ptr<> will refer to nothing). It makes the initialisation code a little easier to maintain than trying to track the dependencies explicitly.

[AlanGriffiths]

Oh, and a passing thought: your best platform for experimenting with USC is probably Ubuntu 17.04 - this uses USC as part of the Unity8 desktop and you will be able to see the effect of a usr/local install of your build without changing any other components.

[WebDrake]

As for the use of CachePtr - I'm not entirely convinced this is appropriate here. I suspect it exists in USC for historical reasons and is just a copy of an idiom used internally by Mir.

Yes, I had similar suspicions. I'll see if it's possible to remove as a by-product of the MirAL transition.

Oh, and a passing thought: your best platform for experimenting with USC is probably Ubuntu 17.04 - this uses USC as part of the Unity8 desktop and you will be able to see the effect of a usr/local install of your build without changing any other components.

Thanks for that suggestion; until now I had just been running ctest to confirm if I was breaking anything (which obviously won't work for more powerful changes...). 17.04 testing may have to wait a while as I'm on the road right now and don't fancy updating my main development machine yet, but it should be possible to get onto this later in August.

I have some work-in-progress here, in any case:
https://github.com/WebDrake/unity-system-compositor/commits/mirserver-to-miral

The next step from here is to try to unpick the init callback added to the server instance in usc::SystemCompositor::run.

[WebDrake]

@AlanGriffiths I have some questions related to command-line argument handling in MirAL.

Context: usc::Server adds a bunch of custom command-line options under the hood in its constructor (e.g. "version", options related to file descriptors of the display-manager pipe, etc.) and then defines public getter methods for the resulting settings. This is a little non-DRY (you need the option info to be repeated in both the constructor and the getter method) but forgivably so given that it's all wrapped up under the hood in server.cpp. However, all this needs to be unpicked in order to transition to using MirRunner (where the server becomes just an under-the-hood implementation detail).

Part of that transition could be done by just transitioning other parts of the code to use server->get_options, but that only really seems viable as a transitional change. If custom command-line arguments are to be handled by passing in CommandLineOption instances to MirRunner::run_with in main(), then any coupling between those arguments and other bits of functionality also needs to be done in main().

However, there are a few further complications.

• when called with the --version flag, USC currently outputs version information and exits. It's not obvious how this can be achieved.

  • Perhaps by adding a MirRunner start callback that takes into account a flag set by the CommandLineOption's own callback? If so, how would a start callback indicate that the runner main loop should exit immediately?

• the --blacklist option looks like it could be handled more straightforwardly, just by calling check_blacklist and throwing an exception from within the CommandLineOption callback

  • in the short term it can be handled using get_options directly

  • are there any race conditions I should be aware of which could affect the output of the glGetString calls used in SystemCompositor::run? If not, it looks like there is no reason to not move the blacklist checks to main() as a precursor to adopting the proposed callback solution.

• the --public-socket option looks a little more tricky as its effects combine with other options (--file, --no-file) defined in mirplatform

  • in the short term the easiest setup is probably just to use get_options directly and refactor later to deal with the non-DRY code, after the initial transition to MirRunner is complete

  • this might be done with a class defining a SocketFile class containing a virtual public_socket() method as well as an operator()(mir::Server&). The latter would combine the output of the public_socket method with the checks on "no-file" and "file".

• the display-manager config settings look trickiest to deal with, but maybe a solution along the lines of that already proposed for public socket: have a class defining a virtual the_dm_connection.

Note, handling the --version flag seems like the most immediate blocker, since all others can be dealt with in the short term by non-DRY handling of the command-line arguments. Can you advise particularly on that?

[WebDrake]

are there any race conditions I should be aware of which could affect the output of the glGetString calls used in SystemCompositor::run?

I note that if I take them outside the callback, they consistently return NULL. However, I can't find anywhere in mir or in USC that anything is initialized w.r.t. GLES (I'm not at all familiar with this, so rather flying blind and speculating here).

[AlanGriffiths]

Some of these options are a mystery to me. I'd be inclined to check whether they are actually used (that involves spelunking though startup scripts).

"public-socket" - seems to re-implement (badly) the functionality of "arw-file" with an oddly implemented default of "true". I doubt the option is ever supplied or set to false and it would be easier to simply "setenv("MIR_SERVER_ARW_FILE", "on", 1)" at the top of main(). At least as an interim measure.

AFAICS "version" and "blacklist" can be implemented using miral::CommandLineOption - and that can be used in usc::SystemCompositor::run(). E.g.

void usc::SystemCompositor::run()
{
    CommandLineOption version{
        [](bool is_set) 
        { if (is_set) throw AbnormalExit{string{"unity-system-compositor "} + USC_VERSION}; },
        "version", "Show version of Unity System Compositor"};

    version(*server);

    server->add_init_callback([&]
    ...

The display manager config is a little more involved but basically needs to collect some parameters and then run some startup commands. This can be done with miral::CommandLineOption - note that these execute in the order they are applied to the server (so the earlier options can store the values for use by the later ones). The difficult bit here is Server::the_screen() has dependencies on Mir internals, I don't think that can yet be supported through MirAL.

[AlanGriffiths]

I note that if I take them outside the callback, they consistently return NULL. However, I can't find anywhere in mir or in USC that anything is initialized w.r.t. GLES (I'm not at all familiar with this, so rather flying blind and speculating here).

It happens during initialization, but depends on the platform. E.g. in mgmh::EGLHelper::setup() for mesa.

You need the code to be in an init callback - but you can use miral::CommandLineOption to set that up.

[WebDrake]

if (is_set) throw AbnormalExit{string{"unity-system-compositor "} + USC_VERSION};

I'd considered using an exception like this, but won't this cause the program to exit with EXIT_FAILURE? Even after the transition to MirRunner? (Certainly before.)

[AlanGriffiths]

if (is_set) throw AbnormalExit{string{"unity-system-compositor "} + USC_VERSION};

I'd considered using an exception like this, but won't this cause the program to exit with EXIT_FAILURE? Even after the transition to MirRunner? (Certainly before.)

It will if you just exit with the return code from run_with() (which is what the "blacklist" logic wants anyway).

I was presuming that wouldn't harm anyone using "version" (because I imagine nobody uses it). But you can ignore what run_with() returns if you need to.

[WebDrake]

Yes, for blacklist it's obviously fine. What do you think of this instead for "version" ... ?

    miral::CommandLineOption version_flag{
        [](bool is_set)
        {
            if (is_set)
            {
                std::cerr << "unity-system-compositor " << USC_VERSION << std::endl;
                exit(0);
            }
        },
        "version", "Show version of Unity System Compositor"};

(I'm not sure if the exit(0) will really clean up everything here.)

[AlanGriffiths]

(I'm not sure if the exit(0) will really clean up everything here.)

It (or the kernel) should.

OTOH I avoid exit() unless in main() - it tends to screw with testing. I'd set a flag:

    bool force_exit_ok{false};
    miral::CommandLineOption version_flag{
        [&](bool is_set)
        {
            if (is_set)
            {
                force_exit_ok = true;
                throw AbnormalExit{"..."};
            }
        },
        "version", "Show version of Unity System Compositor"};

[WebDrake]

OK, time for a check-in I think. The latest state of the work can be reviewed in this PR: WebDrake#1

Note that I've deliberately submitted the PR within my own fork, since it's still work-in-progress, and since the master branch here is still missing the most recent patches from upstream (launchpad) USC.

What's been done so far:

• I've managed to entirely remove all custom fields and methods of usc::Server. In particular all the CachedPtr fields and the lazy evaluation using them has been got rid of.

  • this has proven interestingly tricky in a couple of cases, e.g. when initializing the session switcher: there is some real laziness required here, which required a fairly subtle reworking of the calbacks involved. Fortunately I suspect that most of this stuff can be done away with in the long run as we make greater use of the MirAL API.

  • the next steps here are probably to entirely do away with usc::Server and move all of what currently happens in its constructor to the main function

  • with that done, it should be possible to start thinking about replacing various bits of what is done in the constructor with e.g. CommandLineOption and so forth

• usc::SystemCompositor has been reworked as merely the owner of various data structures required by the application; its run method has been replaced with an operator()(mir::Server&), which simply adds an init callback under the hood.

  • it should be straightforward to replace this with an AddInitCallback, but further unpicking is required to minimize direct use of the server API.

  • QUESTION: is it possible to split the one big init callback here into several, added in succession, with the expectation that they will be called in the order in which they are added?

The current extent of testing is that it builds, that ctest passes, and that I can run unity-system-compositor --help without getting any errors. I'll be able to do that some time next week, and I expect some issues will be uncovered in the process.

[WebDrake]

Regarding earlier discussion about how to handle the --version flag: the idea of using a CommandLineOption whose callback throws an AbnormalExit doesn't work. As an example, this is the output that comes out of the existing unity-system-compositor --version:

$ ./bin/unity-system-compositor --version
[2017-08-15 18:20:06.782696] mirplatform: Found graphics driver: throw-on-creation (version 0.26.3)
[2017-08-15 18:20:06.782824] mirplatform: Found graphics driver: mir:stub-graphics (version 0.26.3)
unity-system-compositor 0.9.0

whereas if the opening of main goes like this:

    auto const server = std::make_shared<mir::Server>();

    miral::CommandLineOption version_flag{
        [](bool is_set)
        {
            if (is_set)
            {
                //std::cerr << "unity-system-compositor " << USC_VERSION << std::endl;
                BOOST_THROW_EXCEPTION(
                    mir::AbnormalExit{std::string{"unity-system-compositor "} + USC_VERSION});
            }
        },
        "version", "Show version of Unity System Compositor"
    };

    version_flag(*server);

... then the output is very different:

$ ./bin/unity-system-compositor --version
[2017-08-15 18:25:59.095421] mirplatform: Found graphics driver: throw-on-creation (version 0.26.3)
[2017-08-15 18:25:59.095592] mirplatform: Found graphics driver: mir:stub-graphics (version 0.26.3)
[2017-08-15 18:25:59.097331] mirserver: Starting
[2017-08-15 18:25:59.097523] mircommon: Loading modules from: /usr/lib/x86_64-linux-gnu/mir/server-platform
[2017-08-15 18:25:59.097670] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/graphics-throw.so
[2017-08-15 18:25:59.098010] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/graphics-dummy.so
[2017-08-15 18:25:59.098055] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/input-stub.so
[2017-08-15 18:25:59.098434] mirplatform: Found graphics driver: throw-on-creation (version 0.26.3)
[2017-08-15 18:25:59.098467] mirplatform: Found graphics driver: mir:stub-graphics (version 0.26.3)
[2017-08-15 18:25:59.098629] mirserver: Selected driver: mir:stub-graphics (version 0.26.3)
[2017-08-15 18:25:59.117731] mirserver: Using software cursor
[2017-08-15 18:25:59.119478] mircommon: Loading modules from: /usr/lib/x86_64-linux-gnu/mir/server-platform
[2017-08-15 18:25:59.119592] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/graphics-throw.so
[2017-08-15 18:25:59.119986] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/graphics-dummy.so
[2017-08-15 18:25:59.120090] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/input-stub.so
ERROR: /build/mir-ui6vjS/mir-0.26.3+16.04.20170605/src/server/input/input_probe.cpp(98): Throw in function mir::UniqueModulePtr<mir::input::Platform> mir::input::probe_input_platforms(const mir::options::Option&, const std::shared_ptr<mir::EmergencyCleanupRegistry>&, const std::shared_ptr<mir::input::InputDeviceRegistry>&, const std::shared_ptr<mir::input::InputReport>&, mir::SharedLibraryProberReport&)
Dynamic exception type: boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<std::runtime_error> >
std::exception::what: No appropriate input platform module found

[jsalatas]

and since the master branch here is still missing the most recent patches from upstream

Please notice that you don't commit to master in yunit repos. You commit to upstream which is a clean branch without any debian packaging specifics. Please have a look at the Developr's Howto Wiki https://github.com/yunit-io/documentation/wiki/Developer's-Howto and make sure you understand how git-buildpackage works

[jsalatas]

The latest state of the work can be reviewed in this PR: WebDrake#1

Does it passes the tests or not yet?

[WebDrake]

Please notice that you don't commit to master in yunit repos. You commit to upstream which is a clean branch without any debian packaging specifics.

My comments also apply to upstream; I'm glad that you're separating cleanly between code changes versus packaging-related patches. But on that note, it looks to me like you don't really apply the git buildpackage workflow right: the upstream branch is not vanilla upstream (it has an extra patch in it that could and arguably should be in the master branch instead). That makes it impossible to fast-forward the branch against real upstream changes (in the Launchpad bzr repo).

We should probably discuss that in the other issue I raised, though.

Does it passes the tests or not yet?

All the changes made have been checked against the available unit and integration tests (I've made that a minimum condition for making any commit). However, I think those are likely inadequate to the question of whether the code really works. There looks to be a lot that they don't cover.

A simple example is this commit: WebDrake/unity-system-compositor@c42f366 Originally the spinner = ... line was outside the callback, which failed, as the server->get_options() call can only be made after the apply_settings() call. That wasn't caught by unit or integration tests, but only by me running unity-system-compositor --help and noticing that I was getting an exception thrown.

[jsalatas]

The idea here is that canonical would stop maintaining these packages, so I imported these in git having in mind that we don't need to pull it again from launchpad. Obviously this doesn't seem to be the case so I'm just wondering if it is a good idea to submit your work to launchpad and then I can pull it from there.
:
See also my post in our forum about upstreamed packages
https://forum.yunit.io/viewtopic.php?f=6&t=41#p249

Regarding the tests, if all tests pass but it doesn't actually work for some reason, you probably need to write additional tests to cover that or (if you are busy to do so) open an issue explaining what needs to be done.

[jsalatas]

as a final note, please notice that your mirserver-to-miral branch contains the debian directory (packaging) which we don't want to merge into our upstream. How will you take care of it?

[WebDrake]

Obviously this doesn't seem to be the case so I'm just wondering if it is a good idea to submit your work to launchpad and then I can pull it from there.

Yes, I was thinking the same thing. But bear in mind this situation (of a local patch in an "upstream" branch) also applies to plenty of other yunit-io repos where upstream is actively maintained (e.g. MirAL). So it might be worth trying to set up a branch scheme that supports both use cases (i.e. one where yunit-io is now the maintainer, and ones where there is a real upstream).

Personally I would be inclined to split things into:

• vanilla upstream (no changes at all, can always be fast-forwarded)

• master branch (which includes local patches made as part of the yunit project but not packaging work); PRs can be submitted to this branch, and it can merge from vanilla upstream when changes are made in the real project upstream

• packaging branch (contains the deb-package stuff)

... which looks to me to be quite in line with the git-buildpackage guidelines (certainly with their spirit and intent).

as a final note, please notice that your mirserver-to-miral branch contains the debian directory (packaging) which we don't want to merge into our upstream. How will you take care of it?

I don't think it should be an issue as that would be the original upstream debian directory (which you removed in your Cleaned up upstream patch). So the merge process ought to handle that just fine, and respect your deletion of that directory.

Regarding the tests, if all tests pass but it doesn't actually work for some reason, you probably need to write additional tests to cover that or (if you are busy to do so) open an issue explaining what needs to be done.

I'm happy to try to do so (either write them, or raise an issue) but it's not entirely obvious that these things are readily testable (I rather assume there would already be tests for them if they were). A lot of the issues are concerned with the sequence in which things happen during USC's startup, and hence they involve things like the DBus event loop, etc., where it's not obvious to me how one would mock these things.

@AlanGriffiths I wonder if you could advise on what CI was/is used for USC other than that already defined via ctest?

[jsalatas]

Personally I would be inclined to split things into:

• vanilla upstream (no changes at all, can always be fast-forwarded)

• master branch (which includes local patches made as part of the yunit project but not packaging work); PRs can be submitted to this branch, and it can merge from vanilla upstream when changes are made in the real project upstream

• packaging branch (contains the deb-package stuff)

... which looks to me to be quite in line with the git-buildpackage guidelines (certainly with their spirit and intent).

So another slightly different approach would be to create a new branch (let's call it launchpad) which would be in sync and could be fast-forwarded to launchpad's branch. That branch can be merged to our upstream (ie yunit's cleaned up branch) and from upstream we could have a master as we have now.

Will that work for you?

[jsalatas]

Oh an additional note here: based on my proposal above: we should create PRs to yunit's upstream branch. If for any reason you want to create a PR to launchpad branch then you should do it in yunit project but in launchpad an yunit will pull from there. Right?

[WebDrake]

@jsalatas can I suggest we follow up on the branch question in the dedicated issue? I would rather not further derail the focus of this issue from the mirserver -> miral transition.

The TL;DR is that I'm happy to submit upstream (via Launchpad) or to work out some appropriate way of managing the code here. But I think the point of taking in code is a little way off, so we don't need to address that question now.

[jsalatas]

OK! Please open an issue when you are ready to commit.

[WebDrake]

OK! Please open an issue when you are ready to commit.

I will do so, or else I will announce it here. But in the meantime, any feedback or remarks on the patches already written would be very welcome. Apart from the questions above, this was the main point of checking in ;-)

[WebDrake]

@AlanGriffiths a further question related to MirRunner::run_with and the possibility of using a CommandLineOption for the --version flag.

Just as an experiment, I tried a super-simple "app" that only attempts to implement the --version flag:

#include <miral/command_line_option.h>
#include <miral/runner.h>
#include <iostream>

int main(int argc, char const* argv[])
{
    miral::MirRunner runner{argc, argv, "unity-system-compositor.conf"};

    miral::CommandLineOption version_flag{
        [](bool is_set)
        {
            if (is_set)
            {
                std::cerr << "unity-system-compositor " << USC_VERSION << std::endl;
                exit(0);
            }
        },
        "version", "Show version of Unity System Compositor"
    };

    return runner.run_with({version_flag});
}

Running the resulting executable with the --version flag results in an exception being thrown:

[2017-08-16 00:12:44.241738] mirplatform: Found graphics driver: throw-on-creation (version 0.26.3)
[2017-08-16 00:12:44.241938] mirplatform: Found graphics driver: mir:stub-graphics (version 0.26.3)
[2017-08-16 00:12:44.243312] mirserver: Starting
[2017-08-16 00:12:44.243493] mircommon: Loading modules from: /usr/lib/x86_64-linux-gnu/mir/server-platform
[2017-08-16 00:12:44.243610] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/graphics-throw.so
[2017-08-16 00:12:44.243936] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/graphics-dummy.so
[2017-08-16 00:12:44.243987] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/input-stub.so
[2017-08-16 00:12:44.244440] mirplatform: Found graphics driver: throw-on-creation (version 0.26.3)
[2017-08-16 00:12:44.244481] mirplatform: Found graphics driver: mir:stub-graphics (version 0.26.3)
[2017-08-16 00:12:44.244678] mirserver: Selected driver: mir:stub-graphics (version 0.26.3)
[2017-08-16 00:12:44.265491] mirserver: Using software cursor
[2017-08-16 00:12:44.266242] mircommon: Loading modules from: /usr/lib/x86_64-linux-gnu/mir/server-platform
[2017-08-16 00:12:44.266354] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/graphics-throw.so
[2017-08-16 00:12:44.266732] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/graphics-dummy.so
[2017-08-16 00:12:44.266829] mircommon: Loading module: /usr/lib/x86_64-linux-gnu/mir/server-platform/input-stub.so
ERROR: /build/mir-ui6vjS/mir-0.26.3+16.04.20170605/src/server/input/input_probe.cpp(98): Throw in function mir::UniqueModulePtr<mir::input::Platform> mir::input::probe_input_platforms(const mir::options::Option&, const std::shared_ptr<mir::EmergencyCleanupRegistry>&, const std::shared_ptr<mir::input::InputDeviceRegistry>&, const std::shared_ptr<mir::input::InputReport>&, mir::SharedLibraryProberReport&)
Dynamic exception type: boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<std::runtime_error> >
std::exception::what: No appropriate input platform module found

... which suggests that run_with has some quite particular requirements about what is passed to it. (The instinctive expectation is that, absent any setup of anything that would connect the underlying server to input or displays, it would just handle the command line option and exit.)

I'll dig deeper into this in the next days, but can you advise what's going on here and what is needed in order to address it?

[AlanGriffiths]

@WebDrake it looks as though one thing you are missing is the package mir-graphics-drivers-desktop.

The CI for USC was never to my satisfaction: it depends on manual testing to ensure it worked. Even manual testing was a PITA (I added the --debug-without-dm and --debug-active-session-name options to make it possible to run from the desktop).

You can run USC on X as follows:

$ sudo dbus-run-session unity-system-compositor --debug-without-dm --file /tmp/usc-socket

And connect to it like this:

$ miral-shell --host /tmp/usc-socket

(This latter command will only work a couple of times in "debug" mode because the display manager is stubbed out.)

I'll try to take a deeper look later today, but I may not get time.

PS Canonical isn't maintaining USC, so you needn't worry about "upstream changes".

[WebDrake]

it looks as though one thing you are missing is the package mir-graphics-drivers-desktop.

Ah, thanks! I can confirm that the stub "USC" above works once that is installed. It just outputs a lot more log messages than the real one before it gets to the version info. It also opens up a blank window (without the --version statement, this remains open until it's manually closed): is there any recommended way to avoid this (since with --version we want to just output that info and exit)?

The CI for USC was never to my satisfaction: it depends on manual testing to ensure it worked.

Thanks for confirming what I already strongly suspected.

To take one example: the usc_integration_tests pass for my stub MirRunner-with---version-support program (!). That might be partly because even such a simple MirRunner::run call does a lot of what the integration tests are expecting (difficult to tell without diving deeper than I have so far), but it obviously misses out a lot of what one would want to be validated as working.

You can run USC on X as follows

Cool; I shall have a play with this. Anything in particular I should be watching out for in terms of behaviour? Do I understand right that the second command will run miral-shell using USC as the underlying compositor (effectively, "Mir-on-Mir")?

PS Canonical isn't maintaining USC, so you needn't worry about "upstream changes".

Understood. In the case of USC it's simply about making sure that the yunit-io repo has the latest code from Launchpad.

[WebDrake]

Oh, and:

I'll try to take a deeper look later today, but I may not get time.

Don't worry! I'm having fun with this and I'm not going to be bothered if it takes time to reply to my questions. On the contrary, I'm very grateful for all the generous help you're offering.

[WebDrake]

Regarding testing suggestions: if I run unity-system-compositor --debug-without-dm --file /tmp/usc-socket (whether with or without the sudo dbus-run-session), I wind up with it exiting with an exception as follows:

ERROR: /home/joseph/code/yunit-io/unity-system-compositor/src/dbus_connection_handle.cpp(60): Throw in function void usc::DBusConnectionHandle::request_name(const char*) const
Dynamic exception type: boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<std::runtime_error> >
std::exception::what: dbus_request_name: Connection ":1.110" is not allowed to own the service "com.canonical.Unity.Display" due to security policies in the configuration file

... which appears to be down to a failed call to dbus_bus_request_name.

Going by the remarks here: https://stackoverflow.com/questions/4560877/dbus-bus-request-name-connections-are-not-allowed-to-own-the-service#4561515

... I assume that I must be missing a custom config file in /etc/dbus-1/system.d/ that will specify that com.canonical.Unity service names can be requested.

[AlanGriffiths]

@WebDrake
a super-simple "app" that only attempts to implement the --version flag:

I think MirAL is missing a feature you need. So I've written it:

https://code.launchpad.net/~alan-griffiths/miral/pre_init-CommandLineOption/+merge/329098

[AlanGriffiths]

Regarding testing suggestions: if I run unity-system-compositor --debug-without-dm --file /tmp/usc-socket (whether with or without the sudo dbus-run-session), I wind up with it exiting with an exception as follows:

I need "sudo" to avoid that (but I have USC installed from archive, which may pull in some dbus config).

[WebDrake]

I think MirAL is missing a feature you need. So I've written it:

Oh, fantastic! Thank you so much. Would it help if I tested this locally with my stub app?

[AlanGriffiths]

@WebDrake Would it help if I tested this locally with my stub app?

Yes, it would be nice to confirm it solves your problem.

[WebDrake]

I need "sudo" to avoid that (but I have USC installed from archive, which may pull in some dbus config).

I needed to place the com.canonical.Unity.conf file in /etc/dbus-1/system.d/. I guess if I had actually installed unity-system-compositor (as opposed to just running from the build dir) something along those lines might already have been done.

[WebDrake]

Thus far I've come across two faulty patches that generate segfaults. The first was a premature use of CommandLineOption for the --enable-hardware-cursor flag (it relied on a variable that was out of scope by the time the callback needing it was invoked). This is easy to deal with by just dropping the patch.

The second involved moving how the DM connection was initialized, and looks like it might involve some problems with NullDMMessageHandler. This needs a bit more exploration by the look of things. I'll update the PR once I have fixes in place.

[WebDrake]

The patches in WebDrake#1 have been updated to fix the segfaults. Everything here seems to work as far as

sudo dbus-run-session unity-system-compositor --debug-without-dm --file /tmp/usc-socket
miral-shell --host /tmp/usc-socket

... is concerned. I think the patches could be cleaned up a bit further, however (e.g. the breaking out of code to the separate server_overrides function now seems unnecessary and a premature step).

[WebDrake]

@AlanGriffiths I can confirm that pre_init appears to work nicely. From the current main of my USC:

    auto const config = std::make_shared<mir::Server>();

    miral::CommandLineOption version_flag{
        [](bool is_set)
        {
            if (is_set)
            {
                std::cerr << "unity-system-compositor " << USC_VERSION << std::endl;
                exit(EXIT_SUCCESS);
            }
        },
        "version", "Show version of Unity System Compositor"
    };

    pre_init(version_flag)(*config);

and also from a stub use of MirRunner:

int main(int argc, char const* argv[])
{
    miral::MirRunner runner{argc, argv, "unity-system-compositor.conf"};

    miral::CommandLineOption version_flag{
        [](bool is_set)
        {
            if (is_set)
            {
                std::cerr << "unity-system-compositor " << USC_VERSION << std::endl;
                exit(EXIT_SUCCESS);
            }
        },
        "version", "Show version of Unity System Compositor"
    };

    return runner.run_with({pre_init(version_flag)});
}

... I get the same minimal output, without any windows opening:

$ ./bin/unity-system-compositor --version
[2017-08-16 17:08:51.949393] mirplatform: Found graphics driver: mir:mesa-kms (version 0.26.3)
[2017-08-16 17:08:51.951077] mirplatform: Found graphics driver: mir:mesa-x11 (version 0.26.3)
[2017-08-16 17:08:51.951264] mirplatform: Found graphics driver: throw-on-creation (version 0.26.3)
[2017-08-16 17:08:51.951295] mirplatform: Found graphics driver: mir:stub-graphics (version 0.26.3)
[2017-08-16 17:08:51.952424] mirserver: Starting
unity-system-compositor 0.9.0

... which matches the output of unmodified USC.

I see there's some ongoing discussion about whether to use this feature or make all command-line options pre-init by default. Either way I think it's a useful feature (for example, it will probably be useful in order to separate out the blacklist feature of USC).

[AlanGriffiths]

@WebDrake I had a very quick play with your branch. I installed it and miral trunk on a test system. (There's a gotcha - USC is installed and run from /usr/sbin so the one in /usr/local isn't picked up.)

Anyway, the Unity8 desktop runs on this stack with no obvious problems. So far so good.

[WebDrake]

@AlanGriffiths hah, snap, I was just about to write a summary of where I'm at and ask what next steps might be for testing ;-) Thank you for getting there ahead of me!

I should say thank you to everyone involved for a very clean codebase that's been very easy to refactor. It's been striking how much has been possible to do working fairly 'blind' just on the basis of how the code fits together.

Anyway, if you're happy with things, maybe we should work on getting this into the repo sooner rather than later, so that I don't build up a larger patch backlog than is needed. @jsalatas I'll follow up in the other issue about what would be convenient for submitting a PR against this repo.

Where I'm at, just for the record: I've cleaned up the patchset in WebDrake#1 further as discussed yesterday, including adding a strict MirAL >= 1.5 dependency. I've also taken advantage of the pre_init command-line-option feature in the last patch. This is presumably the version you tested.

I might edit the patches a little further (e.g. to add some helpful comments), but I think this part can be considered pretty much done. Next steps development-wise will be to try to unpick much of the use of add_configuration_option, and to separate out the various parts of the init callback added by usc::SystemCompositor.

From next week I'll have access to a machine that I can put 17.04 on for more in-depth testing. In the meantime, can you advise on any more in-depth things I can do with the USC-on-X testing than just running miral-shell on top of it? I haven't found much more to do with that than just verify that the Ctrl-Alt-Del exit works ;-)

[AlanGriffiths]

@WebDrake as far as USC-on-X I'm not sure there's a lot more you can do. While you could run apps on the miral session using miral-run/miral-xrun but that really just tests Mir-on-(Mir-on-X) and nothing USC specific.

The point of USC is integration with the display manager and repowerd (so that things like suspend/resume are applied correctly). The critical place to test that is a phone image (as that has the more complex lifecycle), but I'm not sure how feasible that is currently.

[WebDrake]

The critical place to test that is a phone image (as that has the more complex lifecycle), but I'm not sure how feasible that is currently.

Sounds like it might be worth involving the UBPorts folks here. @jsalatas are there any concrete plans for how the two projects will share code?

[jsalatas]

Sounds like it might be worth involving the UBPorts folks here.

Forwarded it to UBPorts' developers group.

are there any concrete plans for how the two projects will share code?

Not yet. I guess we will figure it out when yunit on 16.04 using Qt 5.9 is ready (I'm working on this).

[WebDrake]

Forwarded it to UBPorts' developers group.

Cool. Do you have a cross-ref to their issue/forum? Bear in mind they'll need to build the latest trunk MirAL for this to work.

Not yet. I guess we will figure it out when yunit on 16.04 using Qt 5.9 is ready (I'm working on this).

Nice, good luck! I'll probably be AFK from now until Tuesday, so let's catch up on all this in a few days' time.

[jsalatas]

Actually I asked them in their developer's telegram group. No answer/feedback yet. I guess they will follow up here.

[WebDrake]

There's a gotcha - USC is installed and run from /usr/sbin so the one in /usr/local isn't picked up.

So to install my custom USC I'd either need to symlink it (risky I guess) or manually copy it to /usr/sbin?

[jsalatas]

Hmmmm....... I would recommend you to build a deb package and install it normally.

[AlanGriffiths]

Any of those options will work (with corresponding provisos about restoring your system to normal). So long as you understand that a local install doesn't "just work".

[WebDrake]

@AlanGriffiths so /usr/sbin/unity-system-compositor will be preferred over /usr/local/sbin/unity-system-compositor ... ?

[WebDrake]

(In any case, I have things working: this is on a 16.04 system with the Yunit overlay. I used a symlink, for now, just to try things out.)

[WebDrake]

Just to check in — I'll try to be back on this soon (having a busy week). In the meantime, any feedback from the UBPorts folks?

[jsalatas]

Take your time.

No news from UBPorts yet

[WebDrake]

Hello folks -- sorry for the long radio silence. It's been a busy patch. Thank you for the nice blog-post shoutouts in the meantime :-)

Anyway, @AlanGriffiths I've been looking through the code again and considering next steps, and there are a couple of things I was hoping to get your opinion on. Specifically, I'm interested in the bits of code related to setting up the display-manager connection:

            if (server.get_options()->is_set("from-dm-fd") &&
                server.get_options()->is_set("to-dm-fd"))
            {
                dm_connection = std::make_shared<usc::AsioDMConnection>(
                    server.get_options()->get("from-dm-fd", -1),
                    server.get_options()->get("to-dm-fd", -1),
                    the_session_switcher());
            }
            else if (server.get_options()->is_set("debug-without-dm"))
            {
                dm_connection = std::make_shared<NullDMMessageHandler>(
                    the_session_switcher(),
                    server.get_options()->get<std::string>("debug-active-session-name"));
            }
            else
            {
                BOOST_THROW_EXCEPTION(mir::AbnormalExit("to and from FDs are required for display manager"));
            }

What's interesting about this is that it mixes up several concepts: from_dm_fd and to_dm_fd can be either set or not set; and if set, can either have a value, or default to a value of -1. I'm not sure what the consequences of the latter will be (it would require some fairly deep digging into Boost to be certain).

This doesn't seem possible to implement as a conventional CommandLineOption: the optional_value overloads for the latter seem to assume that, if set, a value will actually be provided (i.e. there is no opportunity to set a default value).

The question is whether it is really necessary to worry about that. I've tried to preserve application behaviour precisely so far, but this looks like a case where we could reasonably change things without any obvious impact, as follows:

• debug-without-dm should take precedence over the file-descriptor options (otherwise why is it there?)

• from-dm-fd and to-dm-fd just become regular options with a default value, as does debug-active-session-name.

Thoughts? Unless there are alternative suggestions, I think I'm going to proceed like this.

[AlanGriffiths]

Hi @WebDrake welcome back to activity.

The pre-existing code is misleading. The default value of -1 is spurious: it can never be used as we've already tested is_set(). (Using -1 as a file handle would doubtless fail in some way, but would be pointless replicating.) There's no need to replicate this confusion: the behaviour can be replicated with the Optional overloads.

I'll try to look at your PR later today.

[WebDrake]

Using -1 as a file handle would doubtless fail in some way, but would be pointless replicating.

I found out by running the compositor locally without --debug-without-dm. It actually fails in a relatively informative manner:

ERROR: Throw location unknown (consider using BOOST_THROW_EXCEPTION)
Dynamic exception type: boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::system::system_error> >
std::exception::what: assign: Bad file descriptor

Anyway, I'll rework in line with your suggestion. I actually did use optional_value in the original draft I had of the new code, but wasn't sure if the difference between is_set and having a non-default value was meaningful.