Mir

Merge lp:~alan-griffiths/mir/restrict-access-to-private-headers into lp:mir

Proposed by Alan Griffiths
Status: Merged
Approved by: Alan Griffiths
Approved revision: 2074
Merged at revision: 2094
Proposed branch: lp:~alan-griffiths/mir/restrict-access-to-private-headers
Merge into: lp:mir
Prerequisite: lp:~kdub/mir/no-mirtestdraw
Diff against target: 191 lines (+23/-1)
12 files modified
3rd_party/CMakeLists.txt (+1/-0)
CMakeLists.txt (+1/-1)
benchmarks/frame-uniformity/CMakeLists.txt (+1/-0)
common-ABI-sha1sums (+4/-0)
platform-ABI-sha1sums (+5/-0)
playground/CMakeLists.txt (+1/-0)
server-ABI-sha1sums (+5/-0)
src/CMakeLists.txt (+1/-0)
tests/integration-tests/CMakeLists.txt (+1/-0)
tests/mir_test_doubles/CMakeLists.txt (+1/-0)
tests/mir_test_framework/CMakeLists.txt (+1/-0)
tests/unit-tests/CMakeLists.txt (+1/-0)
To merge this branch: bzr merge lp:~alan-griffiths/mir/restrict-access-to-private-headers
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Alexandros Frantzis (community) Approve
Daniel van Vugt Abstain
Cemil Azizoglu (community) Abstain
Kevin DuBois (community) Approve
Review via email: mp+242097@code.launchpad.net

Commit message

1. "privatized" headers no longer be available throughout Mir codebase. (Especially not examples and acceptance tests.)
2. Some headers re-published as they are required by features in the acceptance tests and examples

Description of the change

1. "privatized" headers no longer be available throughout Mir codebase. (Especially not examples and acceptance tests.)
2. Some headers re-published as they are required by features in the acceptance tests and examples

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

This will keep us more honest about our acceptance tests, so looks good to me.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

[ RUN ] TestClientIPCRender.test_accelerated_render
/tmp/buildd/mir-0.8.0+14.10.20141010bzr2069pkg0vivid181+autopilot0/tests/integration-tests/graphics/android/test_client_render.cpp:404: Failure
Value of: red_pattern.check(*region0)
Actual: false
Expected: true
[ FAILED ] TestClientIPCRender.test_accelerated_render (35 ms)
[ RUN ] TestClientIPCRender.test_accelerated_render_double
/tmp/buildd/mir-0.8.0+14.10.20141010bzr2069pkg0vivid181+autopilot0/tests/integration-tests/graphics/android/test_client_render.cpp:418: Failure
Value of: red_pattern.check(*region0)
Actual: false
Expected: true
/tmp/buildd/mir-0.8.0+14.10.20141010bzr2069pkg0vivid181+autopilot0/tests/integration-tests/graphics/android/test_client_render.cpp:422: Failure
Value of: green_pattern.check(*region1)
Actual: false
Expected: true
[ FAILED ] TestClientIPCRender.test_accelerated_render_double (41 ms)

Looks unrelated - triggering rebuild (logged as lp:/1394180)

Revision history for this message
Kevin DuBois (kdub) wrote :

^^this was a problem in the base branch, remerging the base should resolve

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

We have 3 types of headers :

1- published
2- yet-to-be-published when there is a clear need for a downstream to use it
3- private

Not everyone sees a clear distinction between 2 & 3. The directory src/include is #2 IMO. I don't see a big issue having acceptance tests using these headers. After all, it's not unreasonable to have an API that we might consider publishing but haven't done so yet for whatever reason.

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

I'm slightly uncomfortable with republishing headers no one is using, even indirectly. Rather than decide based on the contents of acceptance tests, I suggest auditing the downstream projects instead, which would then indicate some acceptance tests should not be acceptance tests.

But also this seems like a minor change and the absence of symbols.map changes tells me they're still somewhat private.

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

> We have 3 types of headers :
>
> 1- published
> 2- yet-to-be-published when there is a clear need for a downstream to use it
> 3- private
>
> Not everyone sees a clear distinction between 2 & 3. The directory src/include
> is #2 IMO. I don't see a big issue having acceptance tests using these
> headers. After all, it's not unreasonable to have an API that we might
> consider publishing but haven't done so yet for whatever reason.

We have features that we are committed to supporting and these are tested in the acceptance tests. (That's what acceptance tests are.)

We have example code that shows users how they can make use of the Mir libraries. (That's what examples are.)

So I have a big issue with acceptance tests or examples using headers that are not published. As Kevin says we ought to be honest with our acceptance tests and examples.

I don't have an objection to having "playground" examples and tests of features that are not yet published, but they should not be confused with the above.

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

OK.

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

> I'm slightly uncomfortable with republishing headers no one is using, even
> indirectly. Rather than decide based on the contents of acceptance tests, I
> suggest auditing the downstream projects instead, which would then indicate
> some acceptance tests should not be acceptance tests.

Agreed.

It is clear that Logger ought to be a customization point, so I'm happy with that.

The geometry types are pretty obvious and unlikely to change in problematic ways. There's really no reason to segregate the geometry headers on the basis of which ones happen to be in use right now.

That leaves Clock and BufferWriter:

Clock is only used by benchmarks/frame-uniformity/touch_measuring_client.cpp:21 - so making keeping that private isn't a big deal. Done.

BufferWriter is used by examples/render_overlays.cpp:27 and in stubbing out the graphics "platform". We could demote render_overlays to playground until we have an example that users could and find another way for the tests, but it doesn't seem worth the effort.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> Text conflict in platform-ABI-sha1sums
> Text conflict in server-ABI-sha1sums

The sooner we can dispose of this crutch and use proper ABI checks the better!

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
2074. By Alan Griffiths

merge lp:mir

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

Another stupid sha1sums sync.

Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '3rd_party/CMakeLists.txt'
2--- 3rd_party/CMakeLists.txt 2014-10-29 06:21:10 +0000
3+++ 3rd_party/CMakeLists.txt 2014-11-22 11:23:38 +0000
4@@ -17,6 +17,7 @@
5 set(MIR_INPUT_ANDROID_COMPILE_FLAGS ${MIR_INPUT_ANDROID_COMPILE_FLAGS}
6 PARENT_SCOPE)
7
8+include_directories(${PROJECT_SOURCE_DIR}/src/include/common)
9 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/android-deps)
10 add_subdirectory(android-input)
11
12
13=== modified file 'CMakeLists.txt'
14--- CMakeLists.txt 2014-11-21 20:16:32 +0000
15+++ CMakeLists.txt 2014-11-22 11:23:38 +0000
16@@ -104,7 +104,7 @@
17
18 enable_testing()
19
20-include_directories(include/common src/include/common)
21+include_directories(include/common)
22
23 # Check for boost
24 find_package(Boost 1.48.0 COMPONENTS date_time system program_options iostreams filesystem REQUIRED)
25
26=== modified file 'benchmarks/frame-uniformity/CMakeLists.txt'
27--- benchmarks/frame-uniformity/CMakeLists.txt 2014-11-02 11:35:32 +0000
28+++ benchmarks/frame-uniformity/CMakeLists.txt 2014-11-22 11:23:38 +0000
29@@ -6,6 +6,7 @@
30 ${PROJECT_SOURCE_DIR}/tests/include/
31
32 ${PROJECT_SOURCE_DIR}/src/include/platform
33+ ${PROJECT_SOURCE_DIR}/src/include/common
34 ${PROJECT_SOURCE_DIR}
35 ${MIR_ANDROID_INCLUDE_DIRECTORIES}
36 ${MIR_3RD_PARTY_INCLUDE_DIRECTORIES}
37
38=== modified file 'common-ABI-sha1sums'
39--- common-ABI-sha1sums 2014-11-16 07:58:07 +0000
40+++ common-ABI-sha1sums 2014-11-22 11:23:38 +0000
41@@ -1,6 +1,9 @@
42 3329ada91412ded2f127aee9a92f065e57b81cb2 include/common/mir/cached_ptr.h
43 82ff9499ef62739379616e02164dc98f9914c329 include/common/mir/fd.h
44 b399dc08514751c86e23824612a52378dd9b1e23 include/common/mir/geometry/dimensions.h
45+40eb312d3ca6b0dfcefd647f56a74e77e10d8920 include/common/mir/geometry/displacement.h
46+283cb0ecf9d544300681fc2ab86bec568e2d20ff include/common/mir/geometry/forward.h
47+bfd87d9fb800c1c1b528c00185570abac569caf2 include/common/mir/geometry/length.h
48 d954464ef2d20c2876db68c94512a443186da09b include/common/mir/geometry/point.h
49 dc7c62d3916eec025e8e7deaf57e06077ce38928 include/common/mir/geometry/rectangle.h
50 5161774957e3ca4f5fa4e7db025d0978d2bbef06 include/common/mir/geometry/rectangles.h
51@@ -9,6 +12,7 @@
52 dcf8b8982f138bdde39a241825c610e955cd5e33 include/common/mir/input/input_platform.h
53 208cd6aed5ef5f8f39b3eb86604e4133cb840485 include/common/mir/input/input_receiver_thread.h
54 be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h
55+2fd7274766a92f22e8784cb64af91947ba03d0de include/common/mir/logging/logger.h
56 9ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h
57 9907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h
58 2100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h
59
60=== renamed file 'src/include/common/mir/geometry/displacement.h' => 'include/common/mir/geometry/displacement.h'
61=== renamed file 'src/include/common/mir/geometry/forward.h' => 'include/common/mir/geometry/forward.h'
62=== renamed file 'src/include/common/mir/geometry/length.h' => 'include/common/mir/geometry/length.h'
63=== added directory 'include/common/mir/logging'
64=== renamed file 'src/include/common/mir/logging/logger.h' => 'include/common/mir/logging/logger.h'
65=== modified file 'platform-ABI-sha1sums'
66--- platform-ABI-sha1sums 2014-11-21 12:16:36 +0000
67+++ platform-ABI-sha1sums 2014-11-22 11:23:38 +0000
68@@ -1,6 +1,9 @@
69 3329ada91412ded2f127aee9a92f065e57b81cb2 include/common/mir/cached_ptr.h
70 82ff9499ef62739379616e02164dc98f9914c329 include/common/mir/fd.h
71 b399dc08514751c86e23824612a52378dd9b1e23 include/common/mir/geometry/dimensions.h
72+40eb312d3ca6b0dfcefd647f56a74e77e10d8920 include/common/mir/geometry/displacement.h
73+283cb0ecf9d544300681fc2ab86bec568e2d20ff include/common/mir/geometry/forward.h
74+bfd87d9fb800c1c1b528c00185570abac569caf2 include/common/mir/geometry/length.h
75 d954464ef2d20c2876db68c94512a443186da09b include/common/mir/geometry/point.h
76 dc7c62d3916eec025e8e7deaf57e06077ce38928 include/common/mir/geometry/rectangle.h
77 5161774957e3ca4f5fa4e7db025d0978d2bbef06 include/common/mir/geometry/rectangles.h
78@@ -9,6 +12,7 @@
79 dcf8b8982f138bdde39a241825c610e955cd5e33 include/common/mir/input/input_platform.h
80 208cd6aed5ef5f8f39b3eb86604e4133cb840485 include/common/mir/input/input_receiver_thread.h
81 be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h
82+2fd7274766a92f22e8784cb64af91947ba03d0de include/common/mir/logging/logger.h
83 9ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h
84 9907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h
85 2100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h
86@@ -22,6 +26,7 @@
87 54328767ca330ba166160a486bd0688406ab0222 include/platform/mir/graphics/buffer.h
88 5875f13f5b029a4b5291e9baec3ae434b72be27c include/platform/mir/graphics/buffer_id.h
89 748f5c2aab11189aa02c4a89ab900561548e7304 include/platform/mir/graphics/buffer_properties.h
90+44acfdf6d97069a910e192cba04cd0fe594cd68a include/platform/mir/graphics/buffer_writer.h
91 ffa857f624f1c8cbd50a190107137346fc3204d6 include/platform/mir/graphics/cursor.h
92 fe2b5f13ccd3ec6b78d90d3abbb7d88b48238578 include/platform/mir/graphics/cursor_image.h
93 986f5d0d8ea2c6a42cc320f468e682f81fec46ae include/platform/mir/graphics/display_buffer.h
94
95=== modified file 'playground/CMakeLists.txt'
96--- playground/CMakeLists.txt 2014-10-22 15:11:05 +0000
97+++ playground/CMakeLists.txt 2014-11-22 11:23:38 +0000
98@@ -2,6 +2,7 @@
99 include_directories(
100 ${PROJECT_SOURCE_DIR}/src/include/server
101 ${PROJECT_SOURCE_DIR}/src/include/platform
102+ ${PROJECT_SOURCE_DIR}/src/include/common
103 ${PROJECT_SOURCE_DIR}/include/client
104 ${PROJECT_SOURCE_DIR}/include/server
105 ${PROJECT_SOURCE_DIR}/include/platform
106
107=== modified file 'server-ABI-sha1sums'
108--- server-ABI-sha1sums 2014-11-21 12:16:36 +0000
109+++ server-ABI-sha1sums 2014-11-22 11:23:38 +0000
110@@ -1,6 +1,9 @@
111 3329ada91412ded2f127aee9a92f065e57b81cb2 include/common/mir/cached_ptr.h
112 82ff9499ef62739379616e02164dc98f9914c329 include/common/mir/fd.h
113 b399dc08514751c86e23824612a52378dd9b1e23 include/common/mir/geometry/dimensions.h
114+40eb312d3ca6b0dfcefd647f56a74e77e10d8920 include/common/mir/geometry/displacement.h
115+283cb0ecf9d544300681fc2ab86bec568e2d20ff include/common/mir/geometry/forward.h
116+bfd87d9fb800c1c1b528c00185570abac569caf2 include/common/mir/geometry/length.h
117 d954464ef2d20c2876db68c94512a443186da09b include/common/mir/geometry/point.h
118 dc7c62d3916eec025e8e7deaf57e06077ce38928 include/common/mir/geometry/rectangle.h
119 5161774957e3ca4f5fa4e7db025d0978d2bbef06 include/common/mir/geometry/rectangles.h
120@@ -9,6 +12,7 @@
121 dcf8b8982f138bdde39a241825c610e955cd5e33 include/common/mir/input/input_platform.h
122 208cd6aed5ef5f8f39b3eb86604e4133cb840485 include/common/mir/input/input_receiver_thread.h
123 be7d58c9fde2ce91cc66dd6144b76e08b536266b include/common/mir/int_wrapper.h
124+2fd7274766a92f22e8784cb64af91947ba03d0de include/common/mir/logging/logger.h
125 9ae8473df05dd9e048a73797f01a2f34f7447554 include/common/mir/time/types.h
126 9907751d046e4aea81881cf19e5df52c7a6a813e include/common/mir_toolkit/client_types.h
127 2100c0674d9d882c1845550847357f6a5de5af66 include/common/mir_toolkit/common.h
128@@ -22,6 +26,7 @@
129 54328767ca330ba166160a486bd0688406ab0222 include/platform/mir/graphics/buffer.h
130 5875f13f5b029a4b5291e9baec3ae434b72be27c include/platform/mir/graphics/buffer_id.h
131 748f5c2aab11189aa02c4a89ab900561548e7304 include/platform/mir/graphics/buffer_properties.h
132+44acfdf6d97069a910e192cba04cd0fe594cd68a include/platform/mir/graphics/buffer_writer.h
133 ffa857f624f1c8cbd50a190107137346fc3204d6 include/platform/mir/graphics/cursor.h
134 fe2b5f13ccd3ec6b78d90d3abbb7d88b48238578 include/platform/mir/graphics/cursor_image.h
135 986f5d0d8ea2c6a42cc320f468e682f81fec46ae include/platform/mir/graphics/display_buffer.h
136
137=== modified file 'src/CMakeLists.txt'
138--- src/CMakeLists.txt 2014-11-05 03:25:04 +0000
139+++ src/CMakeLists.txt 2014-11-22 11:23:38 +0000
140@@ -1,3 +1,4 @@
141+include_directories(${PROJECT_SOURCE_DIR}/src/include/common)
142 set(MIR_GENERATED_INCLUDE_DIRECTORIES)
143 add_subdirectory(common/)
144 add_subdirectory(protobuf/)
145
146=== modified file 'tests/integration-tests/CMakeLists.txt'
147--- tests/integration-tests/CMakeLists.txt 2014-11-21 12:16:36 +0000
148+++ tests/integration-tests/CMakeLists.txt 2014-11-22 11:23:38 +0000
149@@ -5,6 +5,7 @@
150 ${PROTOBUF_INCLUDE_DIRS}
151 ${CMAKE_CURRENT_BINARY_DIR}
152 ${PROJECT_SOURCE_DIR}/src/include/platform
153+ ${PROJECT_SOURCE_DIR}/src/include/common
154 ${PROJECT_SOURCE_DIR}/src/include/server
155 ${PROJECT_SOURCE_DIR}/src/include/client
156 )
157
158=== modified file 'tests/mir_test_doubles/CMakeLists.txt'
159--- tests/mir_test_doubles/CMakeLists.txt 2014-11-22 07:29:46 +0000
160+++ tests/mir_test_doubles/CMakeLists.txt 2014-11-22 11:23:38 +0000
161@@ -1,6 +1,7 @@
162 include_directories(
163 ${CMAKE_SOURCE_DIR}
164 ${PROJECT_SOURCE_DIR}/src/include/platform
165+ ${PROJECT_SOURCE_DIR}/src/include/common
166 ${PROJECT_SOURCE_DIR}/src/include/server
167 ${PROJECT_SOURCE_DIR}/src/include/client
168
169
170=== modified file 'tests/mir_test_framework/CMakeLists.txt'
171--- tests/mir_test_framework/CMakeLists.txt 2014-11-21 12:16:36 +0000
172+++ tests/mir_test_framework/CMakeLists.txt 2014-11-22 11:23:38 +0000
173@@ -1,6 +1,7 @@
174 include_directories(
175 ${CMAKE_SOURCE_DIR}
176 ${PROJECT_SOURCE_DIR}/src/include/platform
177+ ${PROJECT_SOURCE_DIR}/src/include/common
178 ${PROJECT_SOURCE_DIR}/src/include/server
179 ${PROJECT_SOURCE_DIR}/src/include/client
180 ${Boost_INCLUDE_DIRS}
181
182=== modified file 'tests/unit-tests/CMakeLists.txt'
183--- tests/unit-tests/CMakeLists.txt 2014-11-21 12:16:36 +0000
184+++ tests/unit-tests/CMakeLists.txt 2014-11-22 11:23:38 +0000
185@@ -5,6 +5,7 @@
186 ${PROJECT_SOURCE_DIR}/src/include/platform
187 ${PROJECT_SOURCE_DIR}/src/include/server
188 ${PROJECT_SOURCE_DIR}/src/include/client
189+ ${PROJECT_SOURCE_DIR}/src/include/common
190 ${GLIB_INCLUDE_DIRS}
191 )
192

Subscribers

People subscribed via source and target branches