Merge lp:~raof/mir/new-dso-versioning-policy into lp:mir

[Chris Halse Rogers]

New DSO versioning guide, plus a new release-checks target to help the release manager follow it.

New DSO versioning guide, plus a release check.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2984
http://jenkins.qa.ubuntu.com/job/mir-ci/5112/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4239
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3146
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4181
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/139/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1266
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1266/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/139/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4182
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4182/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6855
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23884
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/140/console

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I don't have a strong opinion against, but this "unreleased" stuff wasn't part of the discussion I thought this was summarizing. Why has it been added?

review: Needs Information

[Chris Halse Rogers (raof)]

Because while writing up the discussion I thought that doing the unreleased thing would be a good idea, and so I proposed it for discussion here. :)

Specifically - this makes it very clear what to do when introducing new symbols: you add it to MIR_*_unreleased, or create the MIR_*_unreleased stanza if one does not already exist. The scheme proposed in the ML thread solves the problem of “was MIR_CLIENT_9.1 in the latest release, or do I have to bump it?”, but replaces this problem with “have we branched for release yet?”¹. This _unreleased proposal solves both problems.

That then puts a bit more burden on the release manager, which I've attempted to ameliorate with the “release-checks” target.

This *does* mean that we break ABI between development and release, but since we only ship releases, that's fine.

¹: You crazy folk on European time might have a better idea of when this happens than I do, though.

[Chris Halse Rogers (raof)]

Additionally: If we wanted to, I could probably wrangle an abi-check target to fail if the branch changes any ABI *except* by removal and/or addition to an _unreleased stanza. This would then require you to comment out any symbols that you change the ABI of and move them to _unreleased.

For example, adding a new method foo to the graphics::Display interface you'd need to comment out vtable?for?mir::graphics::Display; in MIRPLATFORM_11 and add it to MIRPLATFORM_unreleased in addition to adding the new mir::graphics::Display::foo*; symbol to MIR_PLATFORM_unreleased.

This would be something approximating the append-only ABI specification we've discussed in the past. I think it'd be onerous for us to use at the moment for anything but mirclient, but maybe in the golden vaguely-stable-ABI future...

[Daniel van Vugt (vanvugt)]

Thanks; that's my preferred design too, relating ABIs to Mir releases:
   MIR_CLIENT_0.17 {

Only one gothcha: As this now relates to Mir releases and we hope to not break ABIs every release, some unusual symlinks will be needed when it's implemented:

   libmirclient.so.0.17 -> libmirclient.so.0.18.0
   libmirclient.so.0.18.0 // contains MIR_CLIENT_0.17 and MIR_CLIENT_0.18 symbols

Slightly confusing, but good I think. It's less confusing than all the different number series we maintain today.

[Chris Halse Rogers (raof)]

This does not change the SONAME or filename of any of our libraries.
The MIR_CLIENT_0.17 symbols will reside inside libmirclient.so.9 as
they do now (at least until we break ABI).

[Daniel van Vugt (vanvugt)]

That sounds like a reasonable transition, although a little confusing. We might choose to force an ABI break to complete the transition to the new scheme. Although definitely not till after wily is done and we're sure we're targeting 16.04 only...

[Chris Halse Rogers (raof)]

No more confusing than std::system_error having a symbol version of
GLIBCXX_3.4.11 but being defined in libstdc++.so.6. This is a common
practice.

To be clear, in this proposal when we break mirclient ABI
libmirclient.so.9 will be replaced by libmirclient.so.10.

[Daniel van Vugt (vanvugt)]

If that's true then I disapprove. However since we're talking about the future, I will assume the file names will catch up as I've suggested.

review: Abstain

[Alan Griffiths (alan-griffiths)]

> That then puts a bit more burden on the release manager, which I've attempted
> to ameliorate with the “release-checks” target.

Yes, both in creating the release branch *and* in merging back changes to a stanza that may get updated during the release process.

> ¹: You crazy folk on European time might have a better idea of when this
> happens than I do, though.

I don't think we do.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

ok

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-09-24 15:42:21 +0000
+++ CMakeLists.txt	2015-10-02 08:07:06 +0000
@@ -245,3 +245,11 @@
 add_custom_target(ptest
     COMMAND "${CMAKE_SOURCE_DIR}/tools/run_ctests.sh" "--cost-file" "${CMAKE_BINARY_DIR}/ptest_ctest_cost_data.txt" "sh ${CMAKE_BINARY_DIR}/discover_all_tests.sh" "--" "$$ARGS"
     )
+
+add_custom_target(release-checks)
+
+mir_check_no_unreleased_symbols(mirclient release-checks)
+mir_check_no_unreleased_symbols(mircookie release-checks)
+mir_check_no_unreleased_symbols(mirplatform release-checks)
+mir_check_no_unreleased_symbols(mirprotobuf release-checks)
+mir_check_no_unreleased_symbols(mirserver release-checks)

=== modified file 'cmake/MirCommon.cmake'
--- cmake/MirCommon.cmake	2015-09-17 20:32:32 +0000
+++ cmake/MirCommon.cmake	2015-10-02 08:07:06 +0000
@@ -219,3 +219,29 @@
       "echo \"set_tests_properties(${MAT_NAME} PROPERTIES WORKING_DIRECTORY \\\"${MAT_WORKING_DIRECTORY}\\\")\"\n")
   endif()
 endfunction()
+
+function (mir_check_no_unreleased_symbols TARGET DEPENDENT_TARGET)
+  set(TARGET_NAME "Checking-${TARGET}-for-unreleased-symbols")
+  add_custom_target(
+    ${TARGET_NAME}
+    # Some sort of documentation for this awk monstrosity:
+    #
+    # Objdump format is:
+    #
+    # $ADDRESS $FLAGS $SECTION $SIZE $SYMVER $NAME
+    #
+    # Sadly, $FLAGS is fixed-width but contains 1 or more fields separated by whitespace,
+    # but the rest of the fields are *not* fixed witdth.
+    #
+    # So:
+    #
+    # We look only at the lines which start with a digit; these are the lines containing symbols.
+    # Then, we loop through the fields until we find one matching exactly "D."; this will be the $FLAGS
+    # section with D(ynamic symbol) then some other modifier.
+    # Then we check that the next field ($SECTION) is not *UND* (ie: this symbol is defined in this DSO).
+    # *Then* we check that the $SYMVER field does not match "unreleased"
+    COMMAND /bin/sh -c "objdump -T $<TARGET_FILE:${TARGET}> | awk '/^[[:digit:]]+.*/ { for (i = 2 ; i <= NF ; i++) { if ($i ~ \"^D.\$\") { i++ ; if (\$i != \"*UND*\") { i = i+2 ; if (\$i ~ \"unreleased\") { print ; exit 1}}}}}'"
+    VERBATIM
+  )
+  add_dependencies(${DEPENDENT_TARGET} ${TARGET_NAME})
+endfunction()

=== modified file 'doc/dso_versioning_guide.md'
--- doc/dso_versioning_guide.md	2015-04-28 07:54:10 +0000
+++ doc/dso_versioning_guide.md	2015-10-02 08:07:06 +0000
@@ -6,14 +6,13 @@
 
 There are more detailed descriptions below, but as a general rule:
 
- - If you add a new symbol, add it to a _new_ dotted-decimal version stanza,
-   like `MIR_CLIENT_8.1`, `MIR_CLIENT_8.2`, etc.
- - If you change the behaviour or signature of a symbol and not change SONAME,
-   see "Change symbols without breaking ABI" below
- - If you change SONAME, collect all previous symbol version stanzas into a
-   single labelled with the new SOVER. For example, remove the `MIR_CLIENT_8`,
-   `MIR_CLIENT_8.1`, and `MIR_CLIENT_8.2` stanzas and consolidate their
-   contents into a single new `MIR_CLIENT_9` stanza.
+ - If you add a new symbol, add it to the `*_unreleased` version stanza,
+   like `MIR_CLIENT_unreleased`, `MIR_PLATFORM_unreleased`, etc.
+ - If you change the behaviour or signature of a symbol _and_ wish to preserve
+   backward compatibility, see "Change symbols without breaking ABI" below.
+ - At release time, rename the current `*_unversioned` stanzas to have the
+   version of the current release, like `MIR_CLIENT_0.17`, `MIR_PLATFORM_0.17`,
+   etc.
 
 Can I have some details?
 ------------------------
@@ -89,19 +88,19 @@
 When you add new symbols, add them to a new `version` block in the relevant
 `symbols.map` file, like so:
 
-    MIR_CLIENT_8 {
+    MIR_CLIENT_0.17 {
         global:
             mir_connect_sync;
             ...
             /* Other symbols go here */
     };
 
-    MIR_CLIENT_8.1 {
+    MIR_CLIENT_unreleased {
         global:
             mir_connect_new_symbol;
         local:
             *;
-    } MIR_CLIENT_8;
+    } MIR_CLIENT_0.17;
 
 Note that the script is read top to bottom; wildcards are greedily bound when
 first encountered, so to avoid surprises you should only have a wildcard in the
@@ -133,33 +132,33 @@
 
 `mir_connection_api.cpp`:
 
-    __asm__(".symver old_mir_connection_create_surface,mir_connection_create_surface@MIR_CLIENT_8");
+    __asm__(".symver old_mir_connection_create_surface,mir_connection_create_surface@MIR_CLIENT_0.17");
 
     extern "C" MirWaitHandle* old_mir_connection_create_surface(...)
     /* The old implementation */
 
     /* The @@ specifies that this is the default version */
-    __asm__(".symver mir_connection_create_surface,mir_connection_create_surface@@@MIR_CLIENT_8.1");
+    __asm__(".symver mir_connection_create_surface,mir_connection_create_surface@@@MIR_CLIENT_unreleased");
     MirWaitHandle* mir_connection_create_surface(...)
     /* The new implementation */
 
 `symbols.map`:
 
-    MIR_CLIENT_8 {
+    MIR_CLIENT_0.17 {
         global:
             ...
             mir_connection_create_surface;
             ...
     };
 
-    MIR_CLIENT_8.1 {
+    MIR_CLIENT_unreleased {
         global:
             ...
             mir_connection_create_surface;
             ...
         local:
             *;
-    } MIR_CLIENT_8;
+    } MIR_CLIENT_0.17;
 
 Safely load multiple versions of a library into the same address space
 ----------------------------------------------------------------------