Merge lp:~zyga/checkbox/fix-1476678 into lp:checkbox
- fix-1476678
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Sylvain Pineau |
Approved revision: | 3912 |
Merged at revision: | 3912 |
Proposed branch: | lp:~zyga/checkbox/fix-1476678 |
Merge into: | lp:checkbox |
Diff against target: |
264 lines (+146/-20) 2 files modified
plainbox/plainbox/impl/unit/packaging.py (+92/-20) plainbox/plainbox/impl/unit/test_packging.py (+54/-0) |
To merge this branch: | bzr merge lp:~zyga/checkbox/fix-1476678 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Sylvain Pineau (community) | Approve | ||
Review via email: mp+265506@code.launchpad.net |
Commit message
Description of the change
Zygmunt Krynicki (zyga) wrote : | # |
Zygmunt Krynicki (zyga) wrote : | # |
Currently this crashes for me like this:
python3 manage.py packaging
CRITICAL plainbox.crashes: Executable 'manage.py' invoked with Namespace(
Traceback (most recent call last):
File "/usr/lib/
return self.dispatch_
File "/usr/lib/
return ns.command.
File "/usr/lib/
driver.
File "/usr/lib/
if self.is_
File "/usr/lib/
if (not _strategy_
File "/usr/lib/
and unit.os_version_id == os_release[
KeyError: 'VERSION_ID'
I think it's because 'sid' doesn't have VERSION_ID field. I'll patch the code to fix that.
Zygmunt Krynicki (zyga) wrote : | # |
Yes, this is from sid:
cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux stretch/sid"
NAME="Debian GNU/Linux"
ID=debian
HOME_URL="https:/
SUPPORT_URL="https:/
BUG_REPORT_URL="https:/
Sigh :-) I'll report this as a separate bug. Let's land this
Zygmunt Krynicki (zyga) wrote : | # |
Reported as https:/
Zygmunt Krynicki (zyga) wrote : | # |
I'll do some more testing and review this myself. Separate review is welcome but if you don't want to test this all the way I can do that.
- 3909. By Sylvain Pineau
-
"Release_
2015_Week30 [r=sylvain-pineau][bug=1331302,1341769, 1347120, 1382321, 1383447, 1387782, 1387843, 1388055, 1388747, 1389253, 1399481, 1400646, 1403933, 1406719, 1410501, 1428615, 1451343, 1451541] [author= checkbox- dev]" - 3910. By Zygmunt Krynicki
-
"automatic merge of lp:~zyga/checkbox/fix-1477131/ by tarmac [r=sylvain-pineau][bug=1477131][author=zyga]"
- 3911. By Zygmunt Krynicki
-
plainbox:
unit:packaging: improve documentation Signed-off-by: Zygmunt Krynicki <email address hidden>
- 3912. By Zygmunt Krynicki
-
plainbox:
unit:packaging: fix parsing of Debian meta-data This patch fixes how multiple-entry packaging meta-data units are parsed.
The fix makes the example in the documentation behave as intended.Fixes: https:/
/bugs.launchpad .net/plainbox/ +bug/1476678
Signed-off-by: Zygmunt Krynicki <email address hidden>
Sylvain Pineau (sylvain-pineau) wrote : | # |
+1, thanks for the fix
Preview Diff
1 | === modified file 'plainbox/plainbox/impl/unit/packaging.py' |
2 | --- plainbox/plainbox/impl/unit/packaging.py 2015-07-22 12:57:27 +0000 |
3 | +++ plainbox/plainbox/impl/unit/packaging.py 2015-07-22 13:45:52 +0000 |
4 | @@ -114,6 +114,7 @@ |
5 | import abc |
6 | import errno |
7 | import logging |
8 | +import re |
9 | import sys |
10 | |
11 | from plainbox.i18n import gettext as _ |
12 | @@ -142,12 +143,12 @@ |
13 | |
14 | @property |
15 | def os_id(self): |
16 | - """ Identifier of the operating system. """ |
17 | + """Identifier of the operating system.""" |
18 | return self.get_record_value(self.Meta.fields.os_id) |
19 | |
20 | @property |
21 | def os_version_id(self): |
22 | - """ Version of the operating system. """ |
23 | + """Version of the operating system.""" |
24 | return self.get_record_value(self.Meta.fields.os_version_id) |
25 | |
26 | class Meta: |
27 | @@ -156,7 +157,7 @@ |
28 | |
29 | class fields(SymbolDef): |
30 | |
31 | - """ Symbols for each field of a packaging meta-data unit. """ |
32 | + """Symbols for each field of a packaging meta-data unit.""" |
33 | |
34 | os_id = 'os-id' |
35 | os_version_id = 'os-version-id' |
36 | @@ -174,46 +175,108 @@ |
37 | |
38 | class PackagingDriverError(Exception): |
39 | |
40 | - """ Base for all packaging driver exceptions. """ |
41 | + """Base for all packaging driver exceptions.""" |
42 | |
43 | |
44 | class NoPackagingDetected(PackagingDriverError): |
45 | |
46 | - """ Exception raised when packaging cannot be found. """ |
47 | + """Exception raised when packaging cannot be found.""" |
48 | |
49 | |
50 | class NoApplicableBinaryPackages(PackagingDriverError): |
51 | |
52 | - """ Exception raised when no applicable binary packages are found. """ |
53 | + """Exception raised when no applicable binary packages are found.""" |
54 | |
55 | |
56 | class IPackagingDriver(metaclass=abc.ABCMeta): |
57 | |
58 | - """ Interface for all packaging drivers. """ |
59 | + """Interface for all packaging drivers.""" |
60 | |
61 | @abc.abstractmethod |
62 | def __init__(self, os_release: 'Dict[str, str]'): |
63 | - pass |
64 | + """ |
65 | + Initialize the packaging driver. |
66 | + |
67 | + :param os_release: |
68 | + The dictionary that represents the contents of the |
69 | + ``/etc/os-release`` file. Using this file the packaging driver can |
70 | + infer information about the target operating system that the |
71 | + packaging will be built for. |
72 | + |
73 | + This assumes that packages are built natively, not through a |
74 | + cross-compiler of some sort where the target distribution is |
75 | + different from the host distribution. |
76 | + """ |
77 | |
78 | @abc.abstractmethod |
79 | def inspect_provider(self, provider: 'Provider1') -> None: |
80 | - pass |
81 | + """ |
82 | + Inspect a provider looking for packaging meta-data. |
83 | + |
84 | + :param provider: |
85 | + A provider object to look at. All of the packaging meta-data units |
86 | + there are inspected, if they are applicable (see |
87 | + :meth:`is_applicable()`. Information from applicable units is |
88 | + collected using the :meth:`collect()` method. |
89 | + """ |
90 | |
91 | @abc.abstractmethod |
92 | def is_applicable(self, unit: Unit) -> bool: |
93 | - pass |
94 | + """ |
95 | + Check if the given unit is applicable for collecting. |
96 | + |
97 | + :param unit: |
98 | + The unit to inspect. This doesn't have to be a packaging meta-data |
99 | + unit. In fact, all units are checked with this method. |
100 | + :returns: |
101 | + True if the unit is applicable for collection. |
102 | + |
103 | + Packaging meta-data units that have certain properties are applicable. |
104 | + Refer to the documentation of the module for details. |
105 | + """ |
106 | |
107 | @abc.abstractmethod |
108 | def collect(self, unit: Unit) -> None: |
109 | - pass |
110 | + """ |
111 | + Collect information from the given applicable unit. |
112 | + |
113 | + :param unit: |
114 | + The unit to collect information from. This is usually expressed as |
115 | + additional fields that are specific to the type of native packaging |
116 | + for the system. |
117 | + |
118 | + Collected information is recorded and made available for the |
119 | + :meth:`modify_packaging_tree()` method later. |
120 | + """ |
121 | |
122 | @abc.abstractmethod |
123 | def inspect_packaging(self) -> None: |
124 | - pass |
125 | + """ |
126 | + Inspect the packaging tree for additional information. |
127 | + |
128 | + :raises NoPackagingDetected: |
129 | + Exception raised when packaging cannot be found. |
130 | + :raises NoApplicableBinaryPackages: |
131 | + Exception raised when no applicable binary packages are found. |
132 | + |
133 | + This method looks at the packaging system located in the current |
134 | + directory. This can be the ``debian/`` directory, a particular |
135 | + ``.spec`` file or anything else. Information obtained from the package |
136 | + is used to infer additional properties that can aid in the packaging |
137 | + process. |
138 | + """ |
139 | |
140 | @abc.abstractmethod |
141 | def modify_packaging_tree(self) -> None: |
142 | - pass |
143 | + """ |
144 | + Modify the packaging tree with information from the packaging units. |
145 | + |
146 | + This method uses all of the available information collected from |
147 | + particular packaging meta-data units and from the native packaging to |
148 | + modify the packaging. Additional dependencies may be injected in |
149 | + appropriate places. Please refer to the documentation specific to your |
150 | + packaging system for details. |
151 | + """ |
152 | |
153 | |
154 | def _strategy_id_version(unit, os_release): |
155 | @@ -237,7 +300,7 @@ |
156 | |
157 | class PackagingDriverBase(IPackagingDriver): |
158 | |
159 | - """ Base implementation of a packaging driver. """ |
160 | + """Base implementation of a packaging driver.""" |
161 | |
162 | def __init__(self, os_release: 'Dict[str, str]'): |
163 | self.os_release = os_release |
164 | @@ -262,7 +325,12 @@ |
165 | |
166 | class NullPackagingDriver(PackagingDriverBase): |
167 | |
168 | - """ Null implementation of a packaging driver. """ |
169 | + """ |
170 | + Null implementation of a packaging driver. |
171 | + |
172 | + This driver just does nothing at all. It is used as a fall-back when |
173 | + nothing better is detected. |
174 | + """ |
175 | |
176 | def is_applicable(self, unit: Unit) -> bool: |
177 | return False |
178 | @@ -316,10 +384,14 @@ |
179 | def collect(self, unit: Unit) -> None: |
180 | def rel_list(field): |
181 | relations = unit.get_record_value(field, '').replace('\n', ' ') |
182 | - return ', '.join([rel.strip() for rel in relations.split(',')]) |
183 | - self._depends.append(rel_list('Depends')) |
184 | - self._suggests.append(rel_list('Suggests')) |
185 | - self._recommends.append(rel_list('Recommends')) |
186 | + return ( |
187 | + rel.strip() |
188 | + for rel in re.split(', *', relations) |
189 | + if rel.strip() |
190 | + ) |
191 | + self._depends.extend(rel_list('Depends')) |
192 | + self._suggests.extend(rel_list('Suggests')) |
193 | + self._recommends.extend(rel_list('Recommends')) |
194 | |
195 | def _write_pkg_substvars(self, pkg): |
196 | fname = 'debian/{}.substvars'.format(pkg) |
197 | @@ -357,7 +429,7 @@ |
198 | |
199 | |
200 | def get_packaging_driver() -> IPackagingDriver: |
201 | - """ Get the packaging driver appropriate for the current platform. """ |
202 | + """Get the packaging driver appropriate for the current platform.""" |
203 | if sys.platform.startswith("linux"): |
204 | os_release = get_os_release() |
205 | if (os_release.get('ID') == 'debian' |
206 | |
207 | === added file 'plainbox/plainbox/impl/unit/test_packging.py' |
208 | --- plainbox/plainbox/impl/unit/test_packging.py 1970-01-01 00:00:00 +0000 |
209 | +++ plainbox/plainbox/impl/unit/test_packging.py 2015-07-22 13:45:52 +0000 |
210 | @@ -0,0 +1,54 @@ |
211 | +# This file is part of Checkbox. |
212 | +# |
213 | +# Copyright 2015 Canonical Ltd. |
214 | +# Written by: |
215 | +# Zygmunt Krynicki <zygmunt.krynicki@canonical.com> |
216 | +# |
217 | +# Checkbox is free software: you can redistribute it and/or modify |
218 | +# it under the terms of the GNU General Public License version 3, |
219 | +# as published by the Free Software Foundation. |
220 | +# |
221 | +# Checkbox is distributed in the hope that it will be useful, |
222 | +# but WITHOUT ANY WARRANTY; without even the implied warranty of |
223 | +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
224 | +# GNU General Public License for more details. |
225 | +# |
226 | +# You should have received a copy of the GNU General Public License |
227 | +# along with Checkbox. If not, see <http://www.gnu.org/licenses/>. |
228 | + |
229 | +"""Tests for the PackagingMetaDataUnit and friends.""" |
230 | + |
231 | +from unittest import TestCase |
232 | + |
233 | +from plainbox.impl.unit.packaging import DebianPackagingDriver |
234 | +from plainbox.impl.unit.packaging import PackagingMetaDataUnit |
235 | + |
236 | + |
237 | +class DebianPackagingDriverTests(TestCase): |
238 | + |
239 | + """Tests for the DebianPackagingDriver class.""" |
240 | + |
241 | + def test_fix_1476678(self): |
242 | + """Check https://bugs.launchpad.net/plainbox/+bug/1476678.""" |
243 | + driver = DebianPackagingDriver({}) |
244 | + driver.collect(PackagingMetaDataUnit({ |
245 | + 'Depends': ( |
246 | + 'python3-checkbox-support (>= 0.2),\n' |
247 | + 'python3 (>= 3.2),\n'), |
248 | + 'Recommends': ( |
249 | + 'dmidecode,\n' |
250 | + 'dpkg (>= 1.13),\n' |
251 | + 'lsb-release,\n' |
252 | + 'wodim') |
253 | + })) |
254 | + self.assertEqual(driver._depends, [ |
255 | + 'python3-checkbox-support (>= 0.2)', |
256 | + 'python3 (>= 3.2)', |
257 | + ]) |
258 | + self.assertEqual(driver._recommends, [ |
259 | + 'dmidecode', |
260 | + 'dpkg (>= 1.13)', |
261 | + 'lsb-release', |
262 | + 'wodim' |
263 | + ]) |
264 | + self.assertEqual(driver._suggests, []) |
This is almost, but not quite. I need to see what's missing :-|