Merge ~cjwatson/launchpad-buildd:py3-fix-buildrecipe into launchpad-buildd:master
- Git
- lp:~cjwatson/launchpad-buildd
- py3-fix-buildrecipe
- Merge into master
Proposed by
Colin Watson
Status: | Merged |
---|---|
Approved by: | Colin Watson |
Approved revision: | b61f70289cccfde7585d99abe736b2a00217f331 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~cjwatson/launchpad-buildd:py3-fix-buildrecipe |
Merge into: | launchpad-buildd:master |
Diff against target: |
281 lines (+128/-33) 3 files modified
bin/buildrecipe (+21/-32) debian/changelog (+1/-0) lpbuildd/tests/test_buildrecipe.py (+106/-1) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tom Wardill (community) | Approve | ||
Review via email: mp+391378@code.launchpad.net |
Commit message
Fix bytes/text handling in RecipeBuilder.
Description of the change
To post a comment you must log in.
Revision history for this message
Tom Wardill (twom) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/bin/buildrecipe b/bin/buildrecipe | |||
2 | index f3034e7..b447f01 100755 | |||
3 | --- a/bin/buildrecipe | |||
4 | +++ b/bin/buildrecipe | |||
5 | @@ -14,13 +14,7 @@ import os | |||
6 | 14 | import pwd | 14 | import pwd |
7 | 15 | import socket | 15 | import socket |
8 | 16 | import stat | 16 | import stat |
16 | 17 | from subprocess import ( | 17 | import subprocess |
10 | 18 | PIPE, | ||
11 | 19 | Popen, | ||
12 | 20 | call, | ||
13 | 21 | check_call, | ||
14 | 22 | check_output, | ||
15 | 23 | ) | ||
17 | 24 | import sys | 18 | import sys |
18 | 25 | import tempfile | 19 | import tempfile |
19 | 26 | from textwrap import dedent | 20 | from textwrap import dedent |
20 | @@ -35,18 +29,15 @@ RETCODE_FAILURE_INSTALL_BUILD_DEPS = 202 | |||
21 | 35 | RETCODE_FAILURE_BUILD_SOURCE_PACKAGE = 203 | 29 | RETCODE_FAILURE_BUILD_SOURCE_PACKAGE = 203 |
22 | 36 | 30 | ||
23 | 37 | 31 | ||
25 | 38 | def call_report_rusage(args, env): | 32 | def call_report(args, env): |
26 | 39 | """Run a subprocess. | 33 | """Run a subprocess. |
27 | 40 | 34 | ||
29 | 41 | Report that it was run, and the resources used, and complain if it fails. | 35 | Report that it was run and complain if it fails. |
30 | 42 | 36 | ||
32 | 43 | :return: The process wait status. | 37 | :return: The process exit status. |
33 | 44 | """ | 38 | """ |
34 | 45 | print('RUN %r' % args) | 39 | print('RUN %r' % args) |
39 | 46 | proc = Popen(args, env=env) | 40 | return subprocess.call(args, env=env) |
36 | 47 | pid, status, rusage = os.wait4(proc.pid, 0) | ||
37 | 48 | print(rusage) | ||
38 | 49 | return status | ||
40 | 50 | 41 | ||
41 | 51 | 42 | ||
42 | 52 | class RecipeBuilder: | 43 | class RecipeBuilder: |
43 | @@ -102,31 +93,28 @@ class RecipeBuilder: | |||
44 | 102 | assert not os.path.exists(self.tree_path) | 93 | assert not os.path.exists(self.tree_path) |
45 | 103 | recipe_path = os.path.join(self.work_dir, 'recipe') | 94 | recipe_path = os.path.join(self.work_dir, 'recipe') |
46 | 104 | manifest_path = os.path.join(self.tree_path, 'manifest') | 95 | manifest_path = os.path.join(self.tree_path, 'manifest') |
49 | 105 | recipe_file = open(recipe_path, 'rb') | 96 | with open(recipe_path) as recipe_file: |
48 | 106 | try: | ||
50 | 107 | recipe = recipe_file.read() | 97 | recipe = recipe_file.read() |
51 | 108 | finally: | ||
52 | 109 | recipe_file.close() | ||
53 | 110 | # As of bzr 2.2, a defined identity is needed. In this case, we're | 98 | # As of bzr 2.2, a defined identity is needed. In this case, we're |
54 | 111 | # using buildd@<hostname>. | 99 | # using buildd@<hostname>. |
55 | 112 | hostname = socket.gethostname() | 100 | hostname = socket.gethostname() |
56 | 113 | email = 'buildd@%s' % hostname | 101 | email = 'buildd@%s' % hostname |
58 | 114 | lsb_release = Popen(['/usr/bin/sudo', | 102 | lsb_release = subprocess.Popen(['sudo', |
59 | 115 | '/usr/sbin/chroot', self.chroot_path, 'lsb_release', | 103 | '/usr/sbin/chroot', self.chroot_path, 'lsb_release', |
61 | 116 | '-r', '-s'], stdout=PIPE) | 104 | '-r', '-s'], stdout=subprocess.PIPE, universal_newlines=True) |
62 | 117 | distroseries_version = lsb_release.communicate()[0].rstrip() | 105 | distroseries_version = lsb_release.communicate()[0].rstrip() |
63 | 118 | assert lsb_release.returncode == 0 | 106 | assert lsb_release.returncode == 0 |
64 | 119 | 107 | ||
65 | 120 | if self.git: | 108 | if self.git: |
66 | 121 | print('Git version:') | 109 | print('Git version:') |
71 | 122 | check_call(['git', '--version']) | 110 | subprocess.check_call(['git', '--version']) |
72 | 123 | print(check_output( | 111 | print(subprocess.check_output( |
73 | 124 | ['dpkg-query', '-W', 'git-build-recipe']).rstrip( | 112 | ['dpkg-query', '-W', 'git-build-recipe'], |
74 | 125 | '\n').replace('\t', ' ')) | 113 | universal_newlines=True).rstrip('\n').replace('\t', ' ')) |
75 | 126 | else: | 114 | else: |
76 | 127 | print('Bazaar versions:') | 115 | print('Bazaar versions:') |
79 | 128 | check_call(['bzr', 'version']) | 116 | subprocess.check_call(['bzr', 'version']) |
80 | 129 | check_call(['bzr', 'plugins']) | 117 | subprocess.check_call(['bzr', 'plugins']) |
81 | 130 | 118 | ||
82 | 131 | print('Building recipe:') | 119 | print('Building recipe:') |
83 | 132 | print(recipe) | 120 | print(recipe) |
84 | @@ -149,7 +137,7 @@ class RecipeBuilder: | |||
85 | 149 | '--append-version', '~ubuntu%s.1' % distroseries_version, | 137 | '--append-version', '~ubuntu%s.1' % distroseries_version, |
86 | 150 | recipe_path, self.tree_path, | 138 | recipe_path, self.tree_path, |
87 | 151 | ]) | 139 | ]) |
89 | 152 | retcode = call_report_rusage(cmd, env=env) | 140 | retcode = call_report(cmd, env=env) |
90 | 153 | if retcode != 0: | 141 | if retcode != 0: |
91 | 154 | return retcode | 142 | return retcode |
92 | 155 | (source,) = [name for name in os.listdir(self.tree_path) | 143 | (source,) = [name for name in os.listdir(self.tree_path) |
93 | @@ -217,14 +205,14 @@ class RecipeBuilder: | |||
94 | 217 | file=conf) | 205 | file=conf) |
95 | 218 | ftparchive_env = dict(os.environ) | 206 | ftparchive_env = dict(os.environ) |
96 | 219 | ftparchive_env.pop("APT_CONFIG", None) | 207 | ftparchive_env.pop("APT_CONFIG", None) |
98 | 220 | ret = call( | 208 | ret = subprocess.call( |
99 | 221 | ["apt-ftparchive", "-q=2", "generate", conf_path], | 209 | ["apt-ftparchive", "-q=2", "generate", conf_path], |
100 | 222 | env=ftparchive_env) | 210 | env=ftparchive_env) |
101 | 223 | if ret != 0: | 211 | if ret != 0: |
102 | 224 | return ret | 212 | return ret |
103 | 225 | 213 | ||
104 | 226 | with open(os.path.join(self.apt_dir, "Release"), "w") as release: | 214 | with open(os.path.join(self.apt_dir, "Release"), "w") as release: |
106 | 227 | return call( | 215 | return subprocess.call( |
107 | 228 | ["apt-ftparchive", "-q=2", "-c", conf_path, "release", | 216 | ["apt-ftparchive", "-q=2", "-c", conf_path, "release", |
108 | 229 | self.apt_dir], | 217 | self.apt_dir], |
109 | 230 | stdout=release, env=ftparchive_env) | 218 | stdout=release, env=ftparchive_env) |
110 | @@ -252,7 +240,7 @@ class RecipeBuilder: | |||
111 | 252 | if ret == 0: | 240 | if ret == 0: |
112 | 253 | list_path = os.path.join( | 241 | list_path = os.path.join( |
113 | 254 | self.apt_sources_list_dir, "buildrecipe-archive.list") | 242 | self.apt_sources_list_dir, "buildrecipe-archive.list") |
115 | 255 | return call(['sudo', 'mv', tmp_list_path, list_path]) | 243 | return subprocess.call(['sudo', 'mv', tmp_list_path, list_path]) |
116 | 256 | return ret | 244 | return ret |
117 | 257 | 245 | ||
118 | 258 | def setUpAptArchive(self, package): | 246 | def setUpAptArchive(self, package): |
119 | @@ -296,7 +284,8 @@ class RecipeBuilder: | |||
120 | 296 | print("Running in chroot: %s" % | 284 | print("Running in chroot: %s" % |
121 | 297 | ' '.join("'%s'" % arg for arg in args)) | 285 | ' '.join("'%s'" % arg for arg in args)) |
122 | 298 | sys.stdout.flush() | 286 | sys.stdout.flush() |
124 | 299 | return call(['sudo', '/usr/sbin/chroot', self.chroot_path] + args) | 287 | return subprocess.call( |
125 | 288 | ['sudo', '/usr/sbin/chroot', self.chroot_path] + args) | ||
126 | 300 | 289 | ||
127 | 301 | def copy_in(self, source_path, target_path): | 290 | def copy_in(self, source_path, target_path): |
128 | 302 | """Copy a file into the target environment. | 291 | """Copy a file into the target environment. |
129 | @@ -316,7 +305,7 @@ class RecipeBuilder: | |||
130 | 316 | mode = stat.S_IMODE(os.stat(source_path).st_mode) | 305 | mode = stat.S_IMODE(os.stat(source_path).st_mode) |
131 | 317 | full_target_path = os.path.join( | 306 | full_target_path = os.path.join( |
132 | 318 | self.chroot_path, target_path.lstrip("/")) | 307 | self.chroot_path, target_path.lstrip("/")) |
134 | 319 | check_call( | 308 | subprocess.check_call( |
135 | 320 | ["sudo", "install", "-o", "root", "-g", "root", "-m", "%o" % mode, | 309 | ["sudo", "install", "-o", "root", "-g", "root", "-m", "%o" % mode, |
136 | 321 | source_path, full_target_path]) | 310 | source_path, full_target_path]) |
137 | 322 | 311 | ||
138 | diff --git a/debian/changelog b/debian/changelog | |||
139 | index e46111d..547c167 100644 | |||
140 | --- a/debian/changelog | |||
141 | +++ b/debian/changelog | |||
142 | @@ -1,6 +1,7 @@ | |||
143 | 1 | launchpad-buildd (193) UNRELEASED; urgency=medium | 1 | launchpad-buildd (193) UNRELEASED; urgency=medium |
144 | 2 | 2 | ||
145 | 3 | * Fix handling of bytes arguments passed to BuildManager.runSubProcess. | 3 | * Fix handling of bytes arguments passed to BuildManager.runSubProcess. |
146 | 4 | * Fix bytes/text handling in RecipeBuilder.buildTree. | ||
147 | 4 | 5 | ||
148 | 5 | -- Colin Watson <cjwatson@ubuntu.com> Thu, 24 Sep 2020 16:45:27 +0100 | 6 | -- Colin Watson <cjwatson@ubuntu.com> Thu, 24 Sep 2020 16:45:27 +0100 |
149 | 6 | 7 | ||
150 | diff --git a/lpbuildd/tests/test_buildrecipe.py b/lpbuildd/tests/test_buildrecipe.py | |||
151 | index ba93dfe..a4da180 100644 | |||
152 | --- a/lpbuildd/tests/test_buildrecipe.py | |||
153 | +++ b/lpbuildd/tests/test_buildrecipe.py | |||
154 | @@ -7,6 +7,7 @@ __metaclass__ = type | |||
155 | 7 | 7 | ||
156 | 8 | from contextlib import contextmanager | 8 | from contextlib import contextmanager |
157 | 9 | import imp | 9 | import imp |
158 | 10 | import io | ||
159 | 10 | import os | 11 | import os |
160 | 11 | import shutil | 12 | import shutil |
161 | 12 | import stat | 13 | import stat |
162 | @@ -14,7 +15,10 @@ import sys | |||
163 | 14 | import tempfile | 15 | import tempfile |
164 | 15 | from textwrap import dedent | 16 | from textwrap import dedent |
165 | 16 | 17 | ||
167 | 17 | from fixtures import MockPatchObject | 18 | from fixtures import ( |
168 | 19 | MockPatch, | ||
169 | 20 | MockPatchObject, | ||
170 | 21 | ) | ||
171 | 18 | import six | 22 | import six |
172 | 19 | from systemfixtures import FakeProcesses | 23 | from systemfixtures import FakeProcesses |
173 | 20 | from testtools import TestCase | 24 | from testtools import TestCase |
174 | @@ -82,6 +86,107 @@ class TestRecipeBuilder(TestCase): | |||
175 | 82 | self.resetEnvironment() | 86 | self.resetEnvironment() |
176 | 83 | super(TestRecipeBuilder, self).tearDown() | 87 | super(TestRecipeBuilder, self).tearDown() |
177 | 84 | 88 | ||
178 | 89 | def test_buildTree_git(self): | ||
179 | 90 | def fake_git(args): | ||
180 | 91 | if args["args"][1] == "--version": | ||
181 | 92 | print("git version x.y.z") | ||
182 | 93 | return {} | ||
183 | 94 | else: | ||
184 | 95 | return {"returncode": 1} | ||
185 | 96 | |||
186 | 97 | def fake_git_build_recipe(args): | ||
187 | 98 | print("dummy recipe build") | ||
188 | 99 | os.makedirs(os.path.join(self.builder.tree_path, "foo")) | ||
189 | 100 | return {} | ||
190 | 101 | |||
191 | 102 | processes_fixture = self.useFixture(FakeProcesses()) | ||
192 | 103 | processes_fixture.add( | ||
193 | 104 | lambda _: {"stdout": io.StringIO(u"5.10\n")}, name="sudo") | ||
194 | 105 | processes_fixture.add(fake_git, name="git") | ||
195 | 106 | processes_fixture.add( | ||
196 | 107 | lambda _: {"stdout": io.StringIO(u"git-build-recipe\tx.y.z\n")}, | ||
197 | 108 | name="dpkg-query") | ||
198 | 109 | processes_fixture.add(fake_git_build_recipe, name="git-build-recipe") | ||
199 | 110 | self.builder = RecipeBuilder( | ||
200 | 111 | self.build_id, "Recipe Builder", "builder@example.org>", "grumpy", | ||
201 | 112 | "grumpy", "main", "PPA", git=True) | ||
202 | 113 | with open(os.path.join(self.builder.work_dir, "recipe"), "w") as f: | ||
203 | 114 | f.write("dummy recipe contents\n") | ||
204 | 115 | mock_stdout = six.StringIO() | ||
205 | 116 | self.useFixture(MockPatch("sys.stdout", mock_stdout)) | ||
206 | 117 | self.assertEqual(0, self.builder.buildTree()) | ||
207 | 118 | self.assertEqual( | ||
208 | 119 | os.path.join(self.builder.work_dir_relative, "tree", "foo"), | ||
209 | 120 | self.builder.source_dir_relative) | ||
210 | 121 | expected_recipe_command = [ | ||
211 | 122 | "git-build-recipe", "--safe", "--no-build", | ||
212 | 123 | "--manifest", os.path.join(self.builder.tree_path, "manifest"), | ||
213 | 124 | "--distribution", "grumpy", "--allow-fallback-to-native", | ||
214 | 125 | "--append-version", u"~ubuntu5.10.1", | ||
215 | 126 | os.path.join(self.builder.work_dir, "recipe"), | ||
216 | 127 | self.builder.tree_path, | ||
217 | 128 | ] | ||
218 | 129 | self.assertEqual( | ||
219 | 130 | dedent("""\ | ||
220 | 131 | Git version: | ||
221 | 132 | git version x.y.z | ||
222 | 133 | git-build-recipe x.y.z | ||
223 | 134 | Building recipe: | ||
224 | 135 | dummy recipe contents | ||
225 | 136 | |||
226 | 137 | RUN %s | ||
227 | 138 | dummy recipe build | ||
228 | 139 | """) % repr(expected_recipe_command), | ||
229 | 140 | mock_stdout.getvalue()) | ||
230 | 141 | |||
231 | 142 | def test_buildTree_bzr(self): | ||
232 | 143 | def fake_bzr(args): | ||
233 | 144 | if args["args"][1] == "version": | ||
234 | 145 | print("bzr version x.y.z") | ||
235 | 146 | return {} | ||
236 | 147 | elif args["args"][1] == "plugins": | ||
237 | 148 | print("bzr-plugin x.y.z") | ||
238 | 149 | return {} | ||
239 | 150 | elif "dailydeb" in args["args"][1:]: | ||
240 | 151 | print("dummy recipe build") | ||
241 | 152 | os.makedirs(os.path.join(self.builder.tree_path, "foo")) | ||
242 | 153 | return {} | ||
243 | 154 | else: | ||
244 | 155 | return {"returncode": 1} | ||
245 | 156 | |||
246 | 157 | processes_fixture = self.useFixture(FakeProcesses()) | ||
247 | 158 | processes_fixture.add( | ||
248 | 159 | lambda _: {"stdout": io.StringIO(u"5.10\n")}, name="sudo") | ||
249 | 160 | processes_fixture.add(fake_bzr, name="bzr") | ||
250 | 161 | with open(os.path.join(self.builder.work_dir, "recipe"), "w") as f: | ||
251 | 162 | f.write("dummy recipe contents\n") | ||
252 | 163 | mock_stdout = six.StringIO() | ||
253 | 164 | self.useFixture(MockPatch("sys.stdout", mock_stdout)) | ||
254 | 165 | self.assertEqual(0, self.builder.buildTree()) | ||
255 | 166 | self.assertEqual( | ||
256 | 167 | os.path.join(self.builder.work_dir_relative, "tree", "foo"), | ||
257 | 168 | self.builder.source_dir_relative) | ||
258 | 169 | expected_recipe_command = [ | ||
259 | 170 | "bzr", "-Derror", "dailydeb", "--safe", "--no-build", | ||
260 | 171 | "--manifest", os.path.join(self.builder.tree_path, "manifest"), | ||
261 | 172 | "--distribution", "grumpy", "--allow-fallback-to-native", | ||
262 | 173 | "--append-version", u"~ubuntu5.10.1", | ||
263 | 174 | os.path.join(self.builder.work_dir, "recipe"), | ||
264 | 175 | self.builder.tree_path, | ||
265 | 176 | ] | ||
266 | 177 | self.assertEqual( | ||
267 | 178 | dedent("""\ | ||
268 | 179 | Bazaar versions: | ||
269 | 180 | bzr version x.y.z | ||
270 | 181 | bzr-plugin x.y.z | ||
271 | 182 | Building recipe: | ||
272 | 183 | dummy recipe contents | ||
273 | 184 | |||
274 | 185 | RUN %s | ||
275 | 186 | dummy recipe build | ||
276 | 187 | """) % repr(expected_recipe_command), | ||
277 | 188 | mock_stdout.getvalue()) | ||
278 | 189 | |||
279 | 85 | def test_makeDummyDsc(self): | 190 | def test_makeDummyDsc(self): |
280 | 86 | self.builder.source_dir_relative = os.path.join( | 191 | self.builder.source_dir_relative = os.path.join( |
281 | 87 | self.builder.work_dir_relative, "tree", "foo") | 192 | self.builder.work_dir_relative, "tree", "foo") |