Merge lp:~bjornt/landscape-client/apt-facade-output into lp:~landscape/landscape-client/trunk

Proposed by Björn Tillenius
Status: Merged
Approved by: Thomas Herve
Approved revision: 419
Merged at revision: 409
Proposed branch: lp:~bjornt/landscape-client/apt-facade-output
Merge into: lp:~landscape/landscape-client/trunk
Diff against target: 302 lines (+205/-12)
2 files modified
landscape/package/facade.py (+26/-3)
landscape/package/tests/test_facade.py (+179/-9)
To merge this branch: bzr merge lp:~bjornt/landscape-client/apt-facade-output
Reviewer Review Type Date Requested Status
Thomas Herve (community) Approve
Alberto Donato Approve
Review via email: mp+83138@code.launchpad.net

Description of the change

Capture the fetch and dpkg output and send it back to the server, both
for succesful and failed operations.

The redirection of stdout/stderr isn't directly pretty, but I asked mvo,
and this is they way it's generally done. It's a bit complicated, since
dpkg runs in a subprocess.

To post a comment you must log in.
Revision history for this message
Alberto Donato (ack) wrote :

Looks good, +1!

review: Approve
Revision history for this message
Thomas Herve (therve) wrote :

This is terrible, but not your fault :). +1!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'landscape/package/facade.py'
2--- landscape/package/facade.py 2011-11-22 10:47:13 +0000
3+++ landscape/package/facade.py 2011-11-23 10:50:32 +0000
4@@ -1,5 +1,7 @@
5 import hashlib
6 import os
7+import tempfile
8+from cStringIO import StringIO
9
10 from smart.transaction import (
11 Transaction, PolicyInstall, PolicyUpgrade, PolicyRemove, Failed)
12@@ -363,11 +365,32 @@
13 if dependencies:
14 raise DependencyError(
15 [version for package, version in dependencies])
16+ fetch_output = StringIO()
17+ # Redirect stdout and stderr to a file. We need to work with the
18+ # file descriptors, rather than sys.stdout/stderr, since dpkg is
19+ # run in a subprocess.
20+ fd, install_output_path = tempfile.mkstemp()
21+ old_stdout = os.dup(1)
22+ old_stderr = os.dup(2)
23+ os.dup2(fd, 1)
24+ os.dup2(fd, 2)
25 try:
26- self._cache.commit()
27+ self._cache.commit(
28+ fetch_progress=apt.progress.text.AcquireProgress(fetch_output))
29 except SystemError, error:
30- raise TransactionError(error.args[0])
31- return "ok"
32+ result_text = (
33+ fetch_output.getvalue() + read_file(install_output_path))
34+ raise TransactionError(
35+ error.args[0] + "\n\nPackage operation log:\n" + result_text)
36+ else:
37+ result_text = (
38+ fetch_output.getvalue() + read_file(install_output_path))
39+ finally:
40+ # Restore stdout and stderr.
41+ os.dup2(old_stdout, 1)
42+ os.dup2(old_stderr, 2)
43+ os.remove(install_output_path)
44+ return result_text
45
46 def reset_marks(self):
47 """Clear the pending package operations."""
48
49=== modified file 'landscape/package/tests/test_facade.py'
50--- landscape/package/tests/test_facade.py 2011-11-15 10:04:32 +0000
51+++ landscape/package/tests/test_facade.py 2011-11-23 10:50:32 +0000
52@@ -3,6 +3,7 @@
53 import re
54 import sys
55 import textwrap
56+import tempfile
57
58 from smart.control import Control
59 from smart.cache import Provides
60@@ -30,6 +31,26 @@
61 create_simple_repository)
62
63
64+class FakeOwner(object):
65+ """Fake Owner object that apt.progress.text.AcquireProgress expects."""
66+
67+ def __init__(self, filesize, error_text=""):
68+ self.id = None
69+ self.filesize = filesize
70+ self.complete = False
71+ self.status = None
72+ self.STAT_DONE = object()
73+ self.error_text = error_text
74+
75+
76+class FakeFetchItem(object):
77+ """Fake Item object that apt.progress.text.AcquireProgress expects."""
78+
79+ def __init__(self, owner, description):
80+ self.owner = owner
81+ self.description = description
82+
83+
84 class AptFacadeTest(LandscapeTest):
85
86 helpers = [AptFacadeHelper]
87@@ -713,6 +734,157 @@
88 self.facade.reload_channels()
89 self.assertEqual(self.facade.perform_changes(), None)
90
91+ def test_perform_changes_fetch_progress(self):
92+ """
93+ C{perform_changes()} captures the fetch output and returns it.
94+ """
95+ deb_dir = self.makeDir()
96+ self._add_package_to_deb_dir(deb_dir, "foo")
97+ self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
98+ self.facade.reload_channels()
99+ foo = self.facade.get_packages_by_name("foo")[0]
100+ self.facade.mark_install(foo)
101+ fetch_item = FakeFetchItem(
102+ FakeOwner(1234, error_text="Some error"), "foo package")
103+
104+ def commit(fetch_progress):
105+ fetch_progress.start()
106+ fetch_progress.fetch(fetch_item)
107+ fetch_progress.fail(fetch_item)
108+ fetch_progress.done(fetch_item)
109+ fetch_progress.stop()
110+
111+ self.facade._cache.commit = commit
112+ output = [
113+ line.rstrip()
114+ for line in self.facade.perform_changes().splitlines()
115+ if line.strip()]
116+ self.assertEqual(
117+ ["Get:1 foo package [1234 B]",
118+ "Err foo package",
119+ " Some error",
120+ "Fetched 0 B in 0s (0 B/s)"],
121+ output)
122+
123+ def test_perform_changes_dpkg_output(self):
124+ """
125+ C{perform_changes()} captures the dpkg output and returns it.
126+ """
127+ deb_dir = self.makeDir()
128+ self._add_package_to_deb_dir(deb_dir, "foo")
129+ self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
130+ self.facade.reload_channels()
131+ foo = self.facade.get_packages_by_name("foo")[0]
132+ self.facade.mark_install(foo)
133+
134+ def commit(fetch_progress):
135+ os.write(1, "Stdout output\n")
136+ os.write(2, "Stderr output\n")
137+ os.write(1, "Stdout output again\n")
138+
139+ self.facade._cache.commit = commit
140+ output = [
141+ line.rstrip()
142+ for line in self.facade.perform_changes().splitlines()
143+ if line.strip()]
144+ self.assertEqual(
145+ ["Stdout output", "Stderr output", "Stdout output again"], output)
146+
147+ def test_perform_changes_dpkg_output_error(self):
148+ """
149+ C{perform_changes()} captures the dpkg output and includes it in
150+ the exception message, if committing the cache fails.
151+ """
152+ deb_dir = self.makeDir()
153+ self._add_package_to_deb_dir(deb_dir, "foo")
154+ self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
155+ self.facade.reload_channels()
156+ foo = self.facade.get_packages_by_name("foo")[0]
157+ self.facade.mark_install(foo)
158+
159+ def commit(fetch_progress):
160+ os.write(1, "Stdout output\n")
161+ os.write(2, "Stderr output\n")
162+ os.write(1, "Stdout output again\n")
163+ raise SystemError("Oops")
164+
165+ self.facade._cache.commit = commit
166+ exception = self.assertRaises(
167+ TransactionError, self.facade.perform_changes)
168+ output = [
169+ line.rstrip()
170+ for line in exception.args[0].splitlines()if line.strip()]
171+ self.assertEqual(
172+ ["Oops", "Package operation log:", "Stdout output",
173+ "Stderr output", "Stdout output again"],
174+ output)
175+
176+ def _mock_output_restore(self):
177+ """
178+ Mock methods to ensure that stdout and stderr are restored,
179+ after they have been captured.
180+
181+ Return the path to the tempfile that was used to capture the output.
182+ """
183+ old_stdout = os.dup(1)
184+ old_stderr = os.dup(2)
185+ fd, outfile = tempfile.mkstemp()
186+ mkstemp = self.mocker.replace("tempfile.mkstemp")
187+ mkstemp()
188+ self.mocker.result((fd, outfile))
189+ dup = self.mocker.replace("os.dup")
190+ dup(1)
191+ self.mocker.result(old_stdout)
192+ dup(2)
193+ self.mocker.result(old_stderr)
194+ dup2 = self.mocker.replace("os.dup2")
195+ dup2(old_stdout, 1)
196+ self.mocker.passthrough()
197+ dup2(old_stderr, 2)
198+ self.mocker.passthrough()
199+ return outfile
200+
201+ def test_perform_changes_dpkg_output_reset(self):
202+ """
203+ C{perform_changes()} resets stdout and stderr after the cache commit.
204+ """
205+ deb_dir = self.makeDir()
206+ self._add_package_to_deb_dir(deb_dir, "foo")
207+ self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
208+ self.facade.reload_channels()
209+ foo = self.facade.get_packages_by_name("foo")[0]
210+ self.facade.mark_install(foo)
211+
212+ outfile = self._mock_output_restore()
213+ self.mocker.replay()
214+ self.facade._cache.commit = lambda fetch_progress: None
215+ self.facade.perform_changes()
216+ # Make sure we don't leave the tempfile behind.
217+ self.assertFalse(os.path.exists(outfile))
218+
219+ def test_perform_changes_dpkg_output_reset_error(self):
220+ """
221+ C{perform_changes()} resets stdout and stderr after the cache
222+ commit, even if commit raises an error.
223+ """
224+ deb_dir = self.makeDir()
225+ self._add_package_to_deb_dir(deb_dir, "foo")
226+ self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
227+ self.facade.reload_channels()
228+ foo = self.facade.get_packages_by_name("foo")[0]
229+ self.facade.mark_install(foo)
230+
231+ outfile = self._mock_output_restore()
232+ self.mocker.replay()
233+
234+ def commit(fetch_progress):
235+ raise SystemError("Error")
236+
237+ self.facade._cache.commit = commit
238+ self.assertRaises(TransactionError, self.facade.perform_changes)
239+ # Make sure we don't leave the tempfile behind.
240+ self.assertFalse(os.path.exists(outfile))
241+
242 def test_reset_marks(self):
243 """
244 C{reset_marks()} clears things, so that there's nothing to do
245@@ -791,7 +963,7 @@
246 foo1, foo2 = sorted(self.facade.get_packages_by_name("foo"))
247 self.assertEqual(foo2, foo1.package.candidate)
248 self.facade.mark_install(foo1)
249- self.facade._cache.commit = lambda: None
250+ self.facade._cache.commit = lambda fetch_progress: None
251 self.facade.perform_changes()
252 self.assertEqual(foo1, foo1.package.candidate)
253
254@@ -811,7 +983,6 @@
255 foo1, foo2, foo3 = sorted(self.facade.get_packages_by_name("foo"))
256 self.assertEqual(foo3, foo1.package.candidate)
257 self.facade.mark_upgrade(foo1)
258- self.facade._cache.commit = lambda: None
259 exception = self.assertRaises(
260 DependencyError, self.facade.perform_changes)
261 self.assertEqual([foo3], exception.packages)
262@@ -850,7 +1021,6 @@
263 noauto1.package.mark_auto(False)
264 self.facade.mark_upgrade(auto1)
265 self.facade.mark_upgrade(noauto1)
266- self.facade._cache.commit = lambda: None
267 self.assertRaises(DependencyError, self.facade.perform_changes)
268 self.assertTrue(auto2.package.is_auto_installed)
269 self.assertFalse(noauto2.package.is_auto_installed)
270@@ -862,7 +1032,7 @@
271 """
272 committed = []
273
274- def commit():
275+ def commit(fetch_progress):
276 committed.append(True)
277
278 deb_dir = self.makeDir()
279@@ -887,10 +1057,10 @@
280 self.facade.reload_channels()
281 pkg = self.facade.get_packages_by_name("minimal")[0]
282 self.facade.mark_install(pkg)
283- self.facade._cache.commit = lambda: None
284- # XXX: It should return the Apt output, but that will be done
285- # later.
286- self.assertEqual("ok", self.facade.perform_changes())
287+ self.facade._cache.commit = lambda fetch_progress: None
288+ # An empty string is returned, since we don't call the progress
289+ # objects, which are the ones that build the output string.
290+ self.assertEqual("", self.facade.perform_changes())
291
292 def test_wb_perform_changes_commit_error(self):
293 """
294@@ -903,7 +1073,7 @@
295 [foo] = self.facade.get_packages_by_name("foo")
296 self.facade.mark_remove(foo)
297 cache = self.mocker.replace(self.facade._cache)
298- cache.commit()
299+ cache.commit(fetch_progress=ANY)
300 self.mocker.throw(SystemError("Something went wrong."))
301 self.mocker.replay()
302 exception = self.assertRaises(TransactionError,

Subscribers

People subscribed via source and target branches

to all changes: