Bazaar

Merge lp:~lifeless/bzr/fix-terminal-spew into lp:bzr

  1. fix-terminal-spew
  2. Merge into bzr.dev
Proposed by Robert Collins
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 5310
Proposed branch: lp:~lifeless/bzr/fix-terminal-spew
Merge into: lp:bzr
Diff against target: 414 lines (+204/-59)
7 files modified
NEWS (+17/-2)
bzr (+8/-5)
bzrlib/__init__.py (+120/-43)
bzrlib/smart/medium.py (+5/-5)
bzrlib/trace.py (+3/-3)
bzrlib/ui/__init__.py (+19/-1)
doc/developers/overview.txt (+32/-0)
To merge this branch: bzr merge lp:~lifeless/bzr/fix-terminal-spew
Reviewer Review Type Date Requested Status
Andrew Bennetts Needs Fixing
Review via email: mp+28024@code.launchpad.net

Commit message

Aaron's output fix for bzr from the UDS sprint, tweaked to use a context manager for the whole library. (Aaron Bentley)

Description of the change

Aaron's output fix for bzr from the UDS sprint, tweaked to use a context
manager for the whole library.

To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

It is a little bit odd to have an __exit__ that ignores the exception info passed to it, in the same way that "except: pass" tends to look a bit odd. As you say on IRC, "until we'd behave differently in an __exit__ call" then there's nothing much to do here. Certainly for the 'bzr' front-end we'd expect exceptions to be caught at this point... although if someone did use this in a 'with' statement in 2.5 it would stop e.g. KeyboardInterrupt propagating. So if the exc_* args are not None, I think BzrLibraryState.__exit__ should "return False" to tell 'with' statements to propagate the exception. And pass these arguments to the UI factory's __exit__, obviously... hmm, maybe should return it's true/false value?

I don't think that needs to be fixed before this can land — it's not a regression, and an overall improvement. But there is a small trap lurking here for advanced API users, so if it's not fixed then an XXX would be warranted.

The only other issue is documentation: bzrlib.initialize's docstring needs to describe its return value, and BzrLibraryState's docstring probably ought to mention that it implements the context manager introduced in Python 2.5. Either that, or you should add docstrings to __enter__ or __exit__, but I know which one is easier ;) It's a bit pedantic, but this is now a front door to the bzrlib API so it should be documented thoroughly and clearly.

Finally, I don't think you've addressed Martin[gz]'s concern about moving the UI cleanup earlier: in particular, if the call to sys.exitfunc() (i.e. to run the atexit hooks) causes something to use the UI, what happens? I think some of the -D flags that output end-of-run summaries (e.g. of HPSS call counts, or transport activity), might cause trouble here. Or maybe not, but I'd some reassurance about it :)

review: Needs Fixing
Revision history for this message
Robert Collins (lifeless) wrote :

Re return codes - I'll make it return False everywhere; I would have thought that None == False ?

I'll document it too.

I was going to say that with this change we don't use atexit, but thats a lie.

We use it in medium.py, and I think thats actually a little unsafe. Concretely its _DebugCounter, and it registers atexit so that when gc hasn't happened, when atexit triggers, it can report.

That medium in particular will go boom if its atexit code triggers, because it exists to call into trace.note. (And thats not safe either, because trace.note can change when tests change, so this particular code is very unsafe for unittests - debug counts from one test can report into a different test or even the global ui).

I propose instead that the library state have a cleanup stack (\o/), that we make the library state also get stored to a global by initialise (stopgaps R us), that the test suite set an appropriate library state as part of its log file shenanigans and finally that DebugCounter register with the cleanup stack of the librarystate that it finds.

Revision history for this message
Robert Collins (lifeless) wrote :

Further to this:
 - I'm going to cleanup the test log support stuff; it all fits in neatly.

Exception propogation return values - argh. Seems like cleanups :). If there are N contexts, which ones should we honour the return value of; should we OR/NOT/XOR/AND them?

Revision history for this message
Robert Collins (lifeless) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-06-20 22:55:07 +0000
+++ NEWS 2010-06-21 04:18:27 +0000
@@ -14,11 +14,25 @@
14Compatibility Breaks14Compatibility Breaks
15********************15********************
1616
17* bzrlib library users now need to call ``__enter__`` and ``__exit__`` on
18 the result of ``bzrlib.initialize``. This change was made when fixing
19 the bad habit recent bzr versions have had of leaving progress bars
20 behind on the screen. That required calling another function before
21 exiting the program, and it made sense to provide a full context
22 manager at the same time. (Robert Collins)
23
24* The ``bzr`` front end now requires a ``bzrlib.ui.ui_factory`` which is a
25 context manager in the Python 2.5 and above sense. The bzrlib base class
26 is such a manager, but third party UI factories which do not derive from
27 ``bzrlib.ui.UIFactory`` will be incompatible with the command line front
28 end.
29
17* URLs like ``foo:bar/baz`` are now always parsed as a URL with scheme "foo"30* URLs like ``foo:bar/baz`` are now always parsed as a URL with scheme "foo"
18 and path "bar/baz", even if bzr does not recognize "foo" as a known URL31 and path "bar/baz", even if bzr does not recognize "foo" as a known URL
19 scheme. Previously these URLs would be treated as local paths.32 scheme. Previously these URLs would be treated as local paths.
20 (Gordon Tyler)33 (Gordon Tyler)
2134
35
22New Features36New Features
23************37************
2438
@@ -56,6 +70,8 @@
56 test that all commands available to the test suite have help.70 test that all commands available to the test suite have help.
57 (Robert Collins, #177500)71 (Robert Collins, #177500)
5872
73* Progress output is cleaned up when exiting. (Aaron Bentley)
74
59* Raise ValueError instead of a string exception.75* Raise ValueError instead of a string exception.
60 (John Arbash Meinel, #586926)76 (John Arbash Meinel, #586926)
6177
@@ -181,8 +197,7 @@
181197
182* ``bzr`` does not try to guess the username as ``username@hostname``198* ``bzr`` does not try to guess the username as ``username@hostname``
183 and requires it to be explictly set. This can be set using ``bzr199 and requires it to be explictly set. This can be set using ``bzr
184 whoami``.200 whoami``. (Parth Malwankar, #549310)
185 (Parth Malwankar, #549310)
186201
187* ``bzrlib.commands.Command`` will now raise ValueError during202* ``bzrlib.commands.Command`` will now raise ValueError during
188 construction if there is no __doc__ set. (Note, this will be reverted in203 construction if there is no __doc__ set. (Note, this will be reverted in
189204
=== modified file 'bzr'
--- bzr 2010-06-16 12:47:51 +0000
+++ bzr 2010-06-21 04:18:27 +0000
@@ -135,11 +135,14 @@
135135
136136
137if __name__ == '__main__':137if __name__ == '__main__':
138 bzrlib.initialize()138 library_state = bzrlib.initialize()
139 exit_val = bzrlib.commands.main()139 library_state.__enter__()
140140 try:
141 if profiling:141 exit_val = bzrlib.commands.main()
142 profile_imports.log_stack_info(sys.stderr)142 if profiling:
143 profile_imports.log_stack_info(sys.stderr)
144 finally:
145 library_state.__exit__(None, None, None)
143146
144 # By this point we really have completed everything we want to do, and147 # By this point we really have completed everything we want to do, and
145 # there's no point doing any additional cleanup. Abruptly exiting here148 # there's no point doing any additional cleanup. Abruptly exiting here
146149
=== modified file 'bzrlib/__init__.py'
--- bzrlib/__init__.py 2010-06-16 12:47:51 +0000
+++ bzrlib/__init__.py 2010-06-21 04:18:27 +0000
@@ -14,7 +14,22 @@
14# along with this program; if not, write to the Free Software14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
1616
17"""bzr library"""17"""All of bzr.
18
19Developer documentation is available at
20http://doc.bazaar.canonical.com/bzr.dev/developers/
21
22The project website is at http://bazaar.canonical.com/
23
24Some particularly interesting things in bzrlib are:
25
26 * bzrlib.initialize -- setup the library for use
27 * bzrlib.plugin.load_plugins -- load all installed plugins
28 * bzrlib.branch.Branch.open -- open a branch
29 * bzrlib.workingtree.WorkingTree.open -- open a working tree
30
31We hope you enjoy this library.
32"""
1833
19import time34import time
2035
@@ -114,15 +129,96 @@
114__version__ = _format_version_tuple(version_info)129__version__ = _format_version_tuple(version_info)
115version_string = __version__130version_string = __version__
116131
117132# bzr has various bits of global state that are slowly being eliminated.
118def test_suite():133# This variable is intended to permit any new state-like things to be attached
119 import tests134# to a BzrLibraryState object rather than getting new global variables that
120 return tests.test_suite()135# need to be hunted down. Accessing the current BzrLibraryState through this
121136# variable is not encouraged: it is better to pass it around as part of the
122137# context of an operation than to look it up directly, but when that is too
123def initialize(138# hard, it is better to use this variable than to make a branch new global
124 setup_ui=True,139# variable.
125 stdin=None, stdout=None, stderr=None):140# If using this variable by looking it up (because it can't be easily obtained)
141# it is important to store the reference you get, rather than looking it up
142# repeatedly; that way your code will behave properly in the bzrlib test suite
143# and from programs that do use multiple library contexts.
144global_state = None
145
146
147class BzrLibraryState(object):
148 """The state about how bzrlib has been configured.
149
150 :ivar saved_state: The bzrlib.global_state at the time __enter__ was
151 called.
152 :ivar cleanups: An ObjectWithCleanups which can be used for cleanups that
153 should occur when the use of bzrlib is completed. This is initialised
154 in __enter__ and executed in __exit__.
155 """
156
157 def __init__(self, setup_ui=True, stdin=None, stdout=None, stderr=None):
158 """Create library start for normal use of bzrlib.
159
160 Most applications that embed bzrlib, including bzr itself, should just
161 call bzrlib.initialize(), but it is possible to use the state class
162 directly.
163
164 More options may be added in future so callers should use named
165 arguments.
166
167 BzrLibraryState implements the Python 2.5 Context Manager protocol, and
168 can be used with the with statement. Upon __enter__ the global
169 variables in use by bzr are set, and they are cleared on __exit__.
170
171 :param setup_ui: If true (default) use a terminal UI; otherwise
172 some other ui_factory must be assigned to `bzrlib.ui.ui_factory` by
173 the caller.
174 :param stdin, stdout, stderr: If provided, use these for terminal IO;
175 otherwise use the files in `sys`.
176 """
177 self.setup_ui = setup_ui
178 self.stdin = stdin
179 self.stdout = stdout
180 self.stderr = stderr
181
182 def __enter__(self):
183 # NB: This function tweaks so much global state it's hard to test it in
184 # isolation within the same interpreter. It's not reached on normal
185 # in-process run_bzr calls. If it's broken, we expect that
186 # TestRunBzrSubprocess may fail.
187 if version_info[3] == 'final':
188 from bzrlib.symbol_versioning import suppress_deprecation_warnings
189 suppress_deprecation_warnings(override=True)
190
191 import bzrlib.cleanup
192 import bzrlib.trace
193 self.cleanups = bzrlib.cleanup.ObjectWithCleanups()
194 bzrlib.trace.enable_default_logging()
195
196 if self.setup_ui:
197 import bzrlib.ui
198 stdin = self.stdin or sys.stdin
199 stdout = self.stdout or sys.stdout
200 stderr = self.stderr or sys.stderr
201 bzrlib.ui.ui_factory = bzrlib.ui.make_ui_for_terminal(
202 stdin, stdout, stderr)
203 global global_state
204 self.saved_state = global_state
205 global_state = self
206
207 def __exit__(self, exc_type, exc_val, exc_tb):
208 self.cleanups.cleanup_now()
209 import bzrlib.ui
210 bzrlib.trace._flush_stdout_stderr()
211 bzrlib.trace._flush_trace()
212 import bzrlib.osutils
213 bzrlib.osutils.report_extension_load_failures()
214 bzrlib.ui.ui_factory.__exit__(None, None, None)
215 bzrlib.ui.ui_factory = None
216 global global_state
217 global_state = self.saved_state
218 return False # propogate exceptions.
219
220
221def initialize(setup_ui=True, stdin=None, stdout=None, stderr=None):
126 """Set up everything needed for normal use of bzrlib.222 """Set up everything needed for normal use of bzrlib.
127223
128 Most applications that embed bzrlib, including bzr itself, should call224 Most applications that embed bzrlib, including bzr itself, should call
@@ -131,39 +227,20 @@
131 More options may be added in future so callers should use named arguments.227 More options may be added in future so callers should use named arguments.
132228
133 :param setup_ui: If true (default) use a terminal UI; otherwise 229 :param setup_ui: If true (default) use a terminal UI; otherwise
134 something else must be put into `bzrlib.ui.ui_factory`.230 some other ui_factory must be assigned to `bzrlib.ui.ui_factory` by
231 the caller.
135 :param stdin, stdout, stderr: If provided, use these for terminal IO;232 :param stdin, stdout, stderr: If provided, use these for terminal IO;
136 otherwise use the files in `sys`.233 otherwise use the files in `sys`.
234 :return: A context manager for the use of bzrlib. The __enter__ method of
235 this context needs to be called before it takes effect, and the __exit__
236 should be called by the caller before exiting their process or
237 otherwise stopping use of bzrlib. Advanced callers can use
238 BzrLibraryState directly.
137 """239 """
138 # TODO: mention this in a guide to embedding bzrlib240 return BzrLibraryState(setup_ui=setup_ui, stdin=stdin,
139 #241 stdout=stdout, stderr=stderr)
140 # NB: This function tweaks so much global state it's hard to test it in242
141 # isolation within the same interpreter. It's not reached on normal243
142 # in-process run_bzr calls. If it's broken, we expect that244def test_suite():
143 # TestRunBzrSubprocess may fail.245 import tests
144 246 return tests.test_suite()
145 import atexit
146 import bzrlib.trace
147
148 bzrlib.trace.enable_default_logging()
149 atexit.register(bzrlib.trace._flush_stdout_stderr)
150 atexit.register(bzrlib.trace._flush_trace)
151
152 import bzrlib.ui
153 if stdin is None:
154 stdin = sys.stdin
155 if stdout is None:
156 stdout = sys.stdout
157 if stderr is None:
158 stderr = sys.stderr
159
160 if setup_ui:
161 bzrlib.ui.ui_factory = bzrlib.ui.make_ui_for_terminal(
162 stdin, stdout, stderr)
163
164 if bzrlib.version_info[3] == 'final':
165 from bzrlib.symbol_versioning import suppress_deprecation_warnings
166 suppress_deprecation_warnings(override=True)
167
168 import bzrlib.osutils
169 atexit.register(osutils.report_extension_load_failures)
170247
=== modified file 'bzrlib/smart/medium.py'
--- bzrlib/smart/medium.py 2010-06-16 07:45:53 +0000
+++ bzrlib/smart/medium.py 2010-06-21 04:18:27 +0000
@@ -28,9 +28,9 @@
28import sys28import sys
29import urllib29import urllib
3030
31import bzrlib
31from bzrlib.lazy_import import lazy_import32from bzrlib.lazy_import import lazy_import
32lazy_import(globals(), """33lazy_import(globals(), """
33import atexit
34import socket34import socket
35import thread35import thread
36import weakref36import weakref
@@ -494,16 +494,16 @@
494class _DebugCounter(object):494class _DebugCounter(object):
495 """An object that counts the HPSS calls made to each client medium.495 """An object that counts the HPSS calls made to each client medium.
496496
497 When a medium is garbage-collected, or failing that when atexit functions497 When a medium is garbage-collected, or failing that when
498 are run, the total number of calls made on that medium are reported via498 bzrlib.global_state exits, the total number of calls made on that medium
499 trace.note.499 are reported via trace.note.
500 """500 """
501501
502 def __init__(self):502 def __init__(self):
503 self.counts = weakref.WeakKeyDictionary()503 self.counts = weakref.WeakKeyDictionary()
504 client._SmartClient.hooks.install_named_hook(504 client._SmartClient.hooks.install_named_hook(
505 'call', self.increment_call_count, 'hpss call counter')505 'call', self.increment_call_count, 'hpss call counter')
506 atexit.register(self.flush_all)506 bzrlib.global_state.cleanups.addCleanup(self.flush_all)
507507
508 def track(self, medium):508 def track(self, medium):
509 """Start tracking calls made to a medium.509 """Start tracking calls made to a medium.
510510
=== modified file 'bzrlib/trace.py'
--- bzrlib/trace.py 2010-05-21 06:14:50 +0000
+++ bzrlib/trace.py 2010-06-21 04:18:27 +0000
@@ -374,7 +374,7 @@
374 _trace_file = old_trace_file374 _trace_file = old_trace_file
375 bzr_logger = logging.getLogger('bzr')375 bzr_logger = logging.getLogger('bzr')
376 bzr_logger.removeHandler(new_handler)376 bzr_logger.removeHandler(new_handler)
377 # must be closed, otherwise logging will try to close it atexit, and the377 # must be closed, otherwise logging will try to close it at exit, and the
378 # file will likely already be closed underneath.378 # file will likely already be closed underneath.
379 new_handler.close()379 new_handler.close()
380 bzr_logger.handlers = old_handlers380 bzr_logger.handlers = old_handlers
@@ -540,7 +540,7 @@
540540
541541
542def _flush_stdout_stderr():542def _flush_stdout_stderr():
543 # installed into an atexit hook by bzrlib.initialize()543 # called from the bzrlib library finalizer returned by bzrlib.initialize()
544 try:544 try:
545 sys.stdout.flush()545 sys.stdout.flush()
546 sys.stderr.flush()546 sys.stderr.flush()
@@ -553,7 +553,7 @@
553553
554554
555def _flush_trace():555def _flush_trace():
556 # run from atexit hook556 # called from the bzrlib library finalizer returned by bzrlib.initialize()
557 global _trace_file557 global _trace_file
558 if _trace_file:558 if _trace_file:
559 _trace_file.flush()559 _trace_file.flush()
560560
=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py 2010-03-25 07:34:15 +0000
+++ bzrlib/ui/__init__.py 2010-06-21 04:18:27 +0000
@@ -106,6 +106,9 @@
106 This tells the library how to display things to the user. Through this106 This tells the library how to display things to the user. Through this
107 layer different applications can choose the style of UI.107 layer different applications can choose the style of UI.
108108
109 UI Factories are also context managers, for some syntactic sugar some users
110 need.
111
109 :ivar suppressed_warnings: Identifiers for user warnings that should 112 :ivar suppressed_warnings: Identifiers for user warnings that should
110 no be emitted.113 no be emitted.
111 """114 """
@@ -123,6 +126,22 @@
123 self.suppressed_warnings = set()126 self.suppressed_warnings = set()
124 self._quiet = False127 self._quiet = False
125128
129 def __enter__(self):
130 """Context manager entry support.
131
132 Override in a concrete factory class if initialisation before use is
133 needed.
134 """
135
136 def __exit__(self, exc_type, exc_val, exc_tb):
137 """Context manager exit support.
138
139 Override in a concrete factory class if more cleanup than a simple
140 self.clear_term() is needed when the UIFactory is finished with.
141 """
142 self.clear_term()
143 return False # propogate exceptions.
144
126 def be_quiet(self, state):145 def be_quiet(self, state):
127 """Tell the UI to be more quiet, or not.146 """Tell the UI to be more quiet, or not.
128147
@@ -352,7 +371,6 @@
352 "without an upgrade path.\n" % (inter.target._format,))371 "without an upgrade path.\n" % (inter.target._format,))
353372
354373
355
356class SilentUIFactory(UIFactory):374class SilentUIFactory(UIFactory):
357 """A UI Factory which never prints anything.375 """A UI Factory which never prints anything.
358376
359377
=== modified file 'doc/developers/overview.txt'
--- doc/developers/overview.txt 2010-05-23 20:44:49 +0000
+++ doc/developers/overview.txt 2010-06-21 04:18:27 +0000
@@ -13,6 +13,38 @@
13to the Bazaar mailing list. 13to the Bazaar mailing list.
1414
1515
16Using bzrlib
17############
18
19Within bzr
20==========
21
22When using bzrlib within the ``bzr`` program (for instance as a bzr
23plugin), bzrlib's global state is already available for use.
24
25From outside bzr
26================
27
28To use bzrlib outside of ``bzr`` some global state needs to be setup.
29bzrlib needs ways to handle user input, passwords, a place to emit
30progress bars, logging setup appropriately for your program. The easiest
31way to set all this up in the same fashion ``bzr`` does is to call
32``bzrlib.initialize``. This returns a context manager within which bzrlib
33functions will work correctly. See the pydoc for ``bzrlib.initialize`` for
34more information. In Python 2.4 the ``with`` keyword is not supported and
35so you need to use the context manager manually::
36
37 # This sets up your ~/.bzr.log, ui factory and so on and so forth. It is
38 # not safe to use as a doctest.
39 library_state = bzrlib.initialize()
40 library_state.__enter__()
41 try:
42 pass
43 # do stuff here
44 finally:
45 library_state.__exit__(None, None, None)
46
47
16Core classes48Core classes
17############49############
1850