Mir

Merge lp://qastaging/~vanvugt/mir/deprecate-without-pragmas into lp://qastaging/mir

  1. deprecate-without-pragmas
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Work in progress
Proposed branch: lp://qastaging/~vanvugt/mir/deprecate-without-pragmas
Merge into: lp://qastaging/mir
Diff against target: 362 lines (+29/-40)
17 files modified
include/client/mir/events/event_builders.h (+9/-9)
include/client/mir_toolkit/client_types.h (+1/-1)
include/client/mir_toolkit/events/event.h (+2/-2)
include/client/mir_toolkit/events/input_configuration_event.h (+3/-3)
include/client/mir_toolkit/events/keymap_event.h (+1/-1)
include/client/mir_toolkit/mir_buffer_stream.h (+1/-1)
include/client/mir_toolkit/mir_surface.h (+1/-1)
include/core/mir_toolkit/common.h (+6/-0)
src/client/CMakeLists.txt (+1/-0)
src/client/event.cpp (+0/-6)
src/client/event_printer.cpp (+0/-3)
src/common/CMakeLists.txt (+1/-0)
src/common/events/event.cpp (+0/-6)
src/include/common/mir/events/input_configuration_event.h (+1/-1)
tests/CMakeLists.txt (+2/-0)
tests/acceptance-tests/test_client_surface_events.cpp (+0/-3)
tests/acceptance-tests/test_client_surfaces.cpp (+0/-3)
To merge this branch: bzr merge lp://qastaging/~vanvugt/mir/deprecate-without-pragmas
Reviewer Review Type Date Requested Status
Cemil Azizoglu (community) Abstain
Andreas Pokorny (community) Disapprove
Mir CI Bot continuous-integration Approve
Brandon Schaefer (community) Disapprove
Alan Griffiths Disapprove
Review via email: mp+309532@code.qastaging.launchpad.net

Commit message

Demonstrate how to deprecate functions without requiring pragmas
where we still refer to them internally.

At present only src/client and tests/ are allowed to use deprecated
features. Anywhere else in the tree (or outside the tree) using them
will trigger a compiler error.

Description of the change

This solves the scaling problem of:
https://code.launchpad.net/~mir-team/mir/pragma-deprecated-functions/+merge/309523

PROS:
* Less code, less diff required when deprecating functions.
* Easier to read.
* Portable to other compilers (by not using GCC extensions).
* No pragmas.
* Quicker and easier to verify a whole subdirectory is free from deprecated functionality.

CONS:
* Can only override deprecation per subdirectory or per file, not per-function within a file.

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3791
https://mir-jenkins.ubuntu.com/job/mir-ci/2075/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2662
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2725
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2717
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2717
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2717
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2691
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2691/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2691
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2691/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2691
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2691/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2691
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2691/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2691
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2691/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2691
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2691/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2075/rebuild

review: Approve (continuous-integration)
Reply
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

There are three cases to consider:

1. the implementation of a deprecated feature should compile (and will be deleted).
2. a test of a deprecated feature should compile (and will be deleted).
3. an "internal" use of a deprecated feature should /not/ compile "silently".

Your approach makes it much harder to identify and address case 3. An "internal" use should either be migrated to the proposed alternative or "flagged" (e.g. with a pragma) for migration *prior* to deleting the implementation.

First flagging everything with pragmas may be "noisier", but it gets us to a better place for managing migration.

review: Disapprove
Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I actually covered all three of those points.

Use of deprecated features should indeed not compile in places other than where they are implemented and tested (or called internally). Using this approach actually would trigger a warning/error to be flagged in examples/ and playground/.

This approach does not make it harder to identify case 3. Use of deprecated functions in examples/ while they still exist will cause a compiler warning/error right now. And of course when a function is deleted will cause compilation errors too.

Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

An additional benefit to this approach is that it moves us in a direction that makes it easier to support non-gcc compilers/directives.

Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

If you wanted to for example find the tests using deprecated features before they were deleted, you simply remove one line from tests/CMakeLists.txt:
   add_definitions(-DMIR_USE_DEPRECATED_APIS)

So this approach is more flexible, more scalable and easier to read. I don't think you have much of an argument against it.

Reply
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> If you wanted to for example find the tests using deprecated features before
> they were deleted, you simply remove one line from tests/CMakeLists.txt:
> add_definitions(-DMIR_USE_DEPRECATED_APIS)

...and insert pragmas for all the uses I'm not addressing. Which brings "me" to the approach Brandon has already proposed.

The same applies to src/client.

> So this approach is more flexible, more scalable and easier to read. I don't
> think you have much of an argument against it.

So this approach simply introduces steps for adding and removing a define. I don't think you have much of an argument for it.

Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

That's not the plan. You don't then insert pragmas, you instead move the placement of #define MIR_USE_DEPRECATED_APIS to a more and more refined position, until there are no instances remaining.

But now you're picking on unrealistic use cases and arguing for Brandon's branch which solves the problem with two orders of magnitude more code and effort than I propose. And you propose something that's harder to read and portable to fewer compilers.

Reply
Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

My other branch was also incorrect in attempting to deprecate things that didnt have a function that existed. My next branch will deprecate a smaller sub set of things, and only #pragma on tests/examples while fixing all src/.

Reply
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I'm sure none of us enjoy such disagreements.

Fortunately this can be resolved just by a few more people voting here.

Reply
Revision history for this message
Brandon Schaefer (brandontschaefer) :
review: Disapprove
Reply
lp://qastaging/~vanvugt/mir/deprecate-without-pragmas updated
3792. By Daniel van Vugt

Merge latest trunk

3793. By Daniel van Vugt

Merge latest trunk

3794. By Daniel van Vugt

Convert deprecations that landed yesterday

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3794
https://mir-jenkins.ubuntu.com/job/mir-ci/2108/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2711/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2774
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2766
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2766
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2766
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2740/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2740/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2740
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2740/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2740/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2740/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2740/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2108/rebuild

review: Needs Fixing (continuous-integration)
Reply
lp://qastaging/~vanvugt/mir/deprecate-without-pragmas updated
3795. By Daniel van Vugt

Fix build failure

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

If you're disapproving, please comment or add your reasons to the CONS list up in the description field.

Reply
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3795
https://mir-jenkins.ubuntu.com/job/mir-ci/2109/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2712
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2775
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2767
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2767
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2767
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2741
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2741/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2741
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2741/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2741
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2741/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2741
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2741/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2741
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2741/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2741
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2741/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2109/rebuild

review: Approve (continuous-integration)
Reply
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

So we would have to use a different set of compile parameters for different parts of our code. I think we should not do that in general.

But what I think makes this approach less useful is that we hide the users of deprecated features in our code and the hiding is general and not per feature or group of functions. I see us reworking our code with alternatives and resolve deprecated parts one feature at a time.

review: Disapprove
Reply
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

As far as I'm concerned, both methods have downsides and upsides and one is not a clear winner over the other.

review: Abstain
Reply

Unmerged revisions

3795. By Daniel van Vugt

Fix build failure

3794. By Daniel van Vugt

Convert deprecations that landed yesterday

3793. By Daniel van Vugt

Merge latest trunk

3792. By Daniel van Vugt

Merge latest trunk

3791. By Daniel van Vugt

Remove more pragmas

3790. By Daniel van Vugt

Demonstrate how to deprecate functions without requiring pragmas
where we still refer to them internally.

This solves the scaling problem of:
https://code.launchpad.net/~mir-team/mir/pragma-deprecated-functions/+merge/309523

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches