Merge lp:~robert-ancell/mir/server-headers into lp:~mir-team/mir/trunk

[Robert Ancell]

Install the server headers so that Mir compositors can be developed separately from lp:mir

Install the server headers so that Mir compositors can be developed separately from lp:mir

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:498
http://jenkins.qa.ubuntu.com/job/mir-ci/77/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/78/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/78/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/77/rebuild

review: Approve (continuous-integration)

[Thomas Voß (thomas-voss)]

Looks good, a minor niggle: Could you rename SERVER_PUBLIC_HEADERS to MIR_SERVER_PUBLIC_HEADERS to match the naming pattern?

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Is this a full or partial resolution for bug 1136938?

[Daniel van Vugt (vanvugt)]

P.S. I think Thomas is wrong. SERVER_PUBLIC_HEADERS is a local variable. It does not need a namespace "MIR_" prefix.

[Daniel van Vugt (vanvugt)]

I used to distinguish locals by making them lower case. We could start doing that...

[Robert Ancell (robert-ancell)]

Yes, it does fix bug 1136938.

[Daniel van Vugt (vanvugt)]

It works well. Stylistically I have no immediate complaint. And bug 1136938 is indeed fixed.

However the list of "server headers" is very long. Some headers such as:
    ${CMAKE_SOURCE_DIR}/include/server/mir/graphics/gl_renderer.h
should not be installed as part of the server (that one is for example code and tests only). There will probably be other such headers that should not be in that list so please re-evaluate the minimum set of headers you really need right now.

review: Needs Fixing

[Robert Ancell (robert-ancell)]

> Looks good, a minor niggle: Could you rename SERVER_PUBLIC_HEADERS to
> MIR_SERVER_PUBLIC_HEADERS to match the naming pattern?

This matches CLIENT_PUBLIC_HEADERS in src/client/CMakeLists.txt so I'll leave that as consistent. If the naming is wrong it should be updated for both in a separate patch.

[Alan Griffiths (alan-griffiths)]

I don't like this being a list of individual files - there's too much chance of it getting out of step.

I would expect everything under either include/common/mir or include/server/mir to be part of this.

IMO if there are things there that shouldn't be exposed then that is a separate issue.

review: Needs Fixing

[Robert Carr (robertcarr)]

I agree, it should use install(DIRECTORY,

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:499
http://jenkins.qa.ubuntu.com/job/mir-ci/104/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/105/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/104/rebuild

review: Needs Fixing (continuous-integration)

[Robert Ancell (robert-ancell)]

Updated, I think this addresses the concerns listed:

Thomas - variable no longer exists
Alan, Robert - whole directory now installed
Daniel - still probably installs headers that are not required, however I agree with Alan here that these headers should not be in include/server if that is the case.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:500
http://jenkins.qa.ubuntu.com/job/mir-ci/105/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/106/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/106/artifact/work/output/*zip*/output.zip
    None: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/834/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/105/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I would expect everything under either include/common/mir or include/server/mir to be part of this.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:500
http://jenkins.qa.ubuntu.com/job/mir-ci/117/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/17/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/118/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/118/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/117/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:502
http://jenkins.qa.ubuntu.com/job/mir-ci/135/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/46/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/136/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/136/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/135/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:502
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/~robert-ancell/mir/server-headers/+merge/154239/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/136/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/47/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/137/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/137/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/136/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Set Needs review ---> Work In Progress --> Needs review
to force Jenkins to try again.

[Daniel van Vugt (vanvugt)]

I would like to say we're done with this issue but there's a new problem. Make install creates:
    include/mir/server/mir/foo/bar.h
You can see two "mir" directories. Users will have to:
    #include <mir/server/mir/foo/bar.h>
which does not look right.

Despite all our previous disagreements about include directories, I *think* it's safe to say the appearance of "mir" twice is a mistake.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

Let us all agree what we are trying to do before trying to fix it in different ways.

I've started a thread on mir-devel - please join us there.

review: Needs Fixing

[Robert Ancell (robert-ancell)]

> I would like to say we're done with this issue but there's a new problem. Make
> install creates:
> include/mir/server/mir/foo/bar.h
> You can see two "mir" directories. Users will have to:
> #include <mir/server/mir/foo/bar.h>
> which does not look right.
>
> Despite all our previous disagreements about include directories, I *think*
> it's safe to say the appearance of "mir" twice is a mistake.

This is not a mistake - the pkg-config file causes you to compile with -I/usr/include/mir/server and a program uses:
     #include <mir/foo.h>

This would be less confusing if the directory was versioned, i.e. /usr/include/mir-0/server or /usr/include/mirserver-0

I'll update the directory as I think that's where we'll end up anyway.

[Robert Ancell (robert-ancell)]

> Let us all agree what we are trying to do before trying to fix it in different
> ways.
>
> I've started a thread on mir-devel - please join us there.

I disagree we need to solve all the details. The point of this merge is you are currently unable to compile against libmirserver as no headers are installed. This merge makes these headers available - their locations and content can be changed at a later date.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:504
http://jenkins.qa.ubuntu.com/job/mir-ci/148/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/63
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/149
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/149/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/148/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Oh I see what you're doing with the pkgconfig include paths now, sorry. That kind of makes sense, and I also see now that _many_ SDKs are installed using the pattern: /usr/include/LIBNAME-VERSION/...

So it works well, assuming you're always using pkgconfig and assuming you accept that "#include <mir/foo/bar.h>" is no longer unique. Because you might have:
   /usr/include/mirserver/mir/foo/bar.h
   /usr/include/mirclient/mir/foo/bar.h

I don't like those two assumptions. They could both be resolved by using "/usr/include/mir/...".

I also don't like that we're installing so many headers, but we can fix that by simply moving things around within the source tree, later.

In the interest of being pragmatic and not holding people up, I think it's good enough for a first step. Even after external projects #include <mir/server/something.h> then we're still able to change the physical location of that "mir" directory without forcing anyone to change their code. So we have no reason to hold this up.

review: Approve

[Robert Ancell (robert-ancell)]

> So it works well, assuming you're always using pkgconfig and assuming you
> accept that "#include <mir/foo/bar.h>" is no longer unique. Because you might
> have:
> /usr/include/mirserver/mir/foo/bar.h
> /usr/include/mirclient/mir/foo/bar.h
>
> I don't like those two assumptions. They could both be resolved by using
> "/usr/include/mir/...".

Correct. I've been mulling that over and I'm not sure if it's a good or bad thing. Note it is generally safe because we're actually copying subdirectories from our source tree so they can't overlap. But yeah, it is something we should consider.

[Alan Griffiths (alan-griffiths)]

I don't think this succeeds in supporting *any* use case - especially the ones we care about.

Specifically, there are a lot of include/server/mir headers that reference mir::geometry, so at the very least /usr/include/mirserver should be merging include/shared/mir.

I still think it better to:

1. first agree what we are trying to support.
2. second agree how we plan to support it.
3. propose changes to make that happen.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:505
http://jenkins.qa.ubuntu.com/job/mir-ci/166/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/83
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/167
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/167/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/166/rebuild

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

Alan, I've installed the common headers now. We can iterate on this later.

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'debian/libmirserver-dev.install'
--- debian/libmirserver-dev.install	2013-02-01 11:08:00 +0000
+++ debian/libmirserver-dev.install	2013-03-21 21:56:22 +0000
@@ -1,2 +1,3 @@
+usr/include/mirserver
 usr/lib/pkgconfig/mirserver.pc
 usr/lib/libmirserver.so

=== modified file 'src/server/CMakeLists.txt'
--- src/server/CMakeLists.txt	2013-03-11 17:42:55 +0000
+++ src/server/CMakeLists.txt	2013-03-21 21:56:22 +0000
@@ -9,6 +9,11 @@
 add_subdirectory(frontend/)
 add_subdirectory(shell/)
 
+set(PREFIX "${CMAKE_INSTALL_PREFIX}")
+set(EXEC_PREFIX "${CMAKE_INSTALL_PREFIX}")
+set(LIBDIR "${CMAKE_INSTALL_PREFIX}/lib")
+set(INCLUDEDIR "${CMAKE_INSTALL_PREFIX}/include/mirserver")
+
 configure_file(
   ${CMAKE_CURRENT_SOURCE_DIR}/mirserver.pc.in
   ${CMAKE_CURRENT_BINARY_DIR}/mirserver.pc
@@ -62,6 +67,15 @@
   install(TARGETS mirserver
     LIBRARY DESTINATION lib
   )
+  install(DIRECTORY
+    ${CMAKE_SOURCE_DIR}/include/server/mir DESTINATION "include/mirserver"
+  )
+  install(DIRECTORY
+    ${CMAKE_SOURCE_DIR}/include/shared/mir DESTINATION "include/mirserver"
+  )
+  install(DIRECTORY
+    ${CMAKE_SOURCE_DIR}/include/shared/mir_toolkit DESTINATION "include/mirserver"
+  )
 else()
   target_link_libraries(mirserver
     LINK_PUBLIC