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

[Robert Collins]

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

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

[lp:~lifeless/bzr/fix-terminal-spew]

5228. By Robert Collins on 2010-06-21

Merge trunk to fix NEWS sections.

[Andrew Bennetts (spiv)]

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

[Robert Collins (lifeless)]

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.

[Robert Collins (lifeless)]

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?

[lp:~lifeless/bzr/fix-terminal-spew]

5229. By Robert Collins on 2010-06-21

Document bzrlib.initialize a little better, and explicitly propogate exceptions in the new __exit__ methods.

5230. By Robert Collins on 2010-06-21

Store the library state as a global variable so that code with no other way of finding the BzrLibraryState can access it.

5231. By Robert Collins on 2010-06-21

Write up some doc about bzrlib.initialize.

5232. By Robert Collins on 2010-06-21

More NEWS about the bzrlib.initialize contract change, and typographical error fixes for __init__.py.

[Robert Collins (lifeless)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-06-20 22:55:07 +0000
+++ NEWS	2010-06-21 04:18:27 +0000
@@ -14,11 +14,25 @@
 Compatibility Breaks
 ********************
 
+* bzrlib library users now need to call ``__enter__`` and ``__exit__`` on
+  the result of ``bzrlib.initialize``. This change was made when fixing
+  the bad habit recent bzr versions have had of leaving progress bars 
+  behind on the screen. That required calling another function before
+  exiting the program, and it made sense to provide a full context
+  manager at the same time. (Robert Collins)
+
+* The ``bzr`` front end now requires a ``bzrlib.ui.ui_factory`` which is a
+  context manager in the Python 2.5 and above sense. The bzrlib base class
+  is such a manager, but third party UI factories which do not derive from
+  ``bzrlib.ui.UIFactory`` will be incompatible with the command line front
+  end.
+  
 * URLs like ``foo:bar/baz`` are now always parsed as a URL with scheme "foo"
   and path "bar/baz", even if bzr does not recognize "foo" as a known URL
   scheme.  Previously these URLs would be treated as local paths.
   (Gordon Tyler)
 
+
 New Features
 ************
 
@@ -56,6 +70,8 @@
   test that all commands available to the test suite have help.
   (Robert Collins, #177500)
 
+* Progress output is cleaned up when exiting.  (Aaron Bentley)
+
 * Raise ValueError instead of a string exception.
   (John Arbash Meinel, #586926)
 
@@ -181,8 +197,7 @@
 
 * ``bzr`` does not try to guess the username as ``username@hostname``
   and requires it to be explictly set. This can be set using ``bzr
-  whoami``.
-  (Parth Malwankar, #549310)
+  whoami``. (Parth Malwankar, #549310)
 
 * ``bzrlib.commands.Command`` will now raise ValueError during
   construction if there is no __doc__ set. (Note, this will be reverted in

=== modified file 'bzr'
--- bzr	2010-06-16 12:47:51 +0000
+++ bzr	2010-06-21 04:18:27 +0000
@@ -135,11 +135,14 @@
 
 
 if __name__ == '__main__':
-    bzrlib.initialize()
-    exit_val = bzrlib.commands.main()
-
-    if profiling:
-        profile_imports.log_stack_info(sys.stderr)
+    library_state = bzrlib.initialize()
+    library_state.__enter__()
+    try:
+        exit_val = bzrlib.commands.main()
+        if profiling:
+            profile_imports.log_stack_info(sys.stderr)
+    finally:
+        library_state.__exit__(None, None, None)
 
     # By this point we really have completed everything we want to do, and
     # there's no point doing any additional cleanup.  Abruptly exiting here

=== 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 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
-"""bzr library"""
+"""All of bzr.
+
+Developer documentation is available at
+http://doc.bazaar.canonical.com/bzr.dev/developers/
+
+The project website is at http://bazaar.canonical.com/
+
+Some particularly interesting things in bzrlib are:
+
+ * bzrlib.initialize -- setup the library for use
+ * bzrlib.plugin.load_plugins -- load all installed plugins
+ * bzrlib.branch.Branch.open -- open a branch
+ * bzrlib.workingtree.WorkingTree.open -- open a working tree
+
+We hope you enjoy this library.
+"""
 
 import time
 
@@ -114,15 +129,96 @@
 __version__ = _format_version_tuple(version_info)
 version_string = __version__
 
-
-def test_suite():
-    import tests
-    return tests.test_suite()
-
-
-def initialize(
-    setup_ui=True,
-    stdin=None, stdout=None, stderr=None):
+# bzr has various bits of global state that are slowly being eliminated.
+# This variable is intended to permit any new state-like things to be attached
+# to a BzrLibraryState object rather than getting new global variables that
+# need to be hunted down. Accessing the current BzrLibraryState through this
+# variable is not encouraged: it is better to pass it around as part of the
+# context of an operation than to look it up directly, but when that is too
+# hard, it is better to use this variable than to make a branch new global
+# variable.
+# If using this variable by looking it up (because it can't be easily obtained)
+# it is important to store the reference you get, rather than looking it up
+# repeatedly; that way your code will behave properly in the bzrlib test suite
+# and from programs that do use multiple library contexts.
+global_state = None
+
+
+class BzrLibraryState(object):
+    """The state about how bzrlib has been configured.
+    
+    :ivar saved_state: The bzrlib.global_state at the time __enter__ was
+        called.
+    :ivar cleanups: An ObjectWithCleanups which can be used for cleanups that
+        should occur when the use of bzrlib is completed. This is initialised
+        in __enter__ and executed in __exit__.
+    """
+
+    def __init__(self, setup_ui=True, stdin=None, stdout=None, stderr=None):
+        """Create library start for normal use of bzrlib.
+
+        Most applications that embed bzrlib, including bzr itself, should just
+        call bzrlib.initialize(), but it is possible to use the state class
+        directly.
+
+        More options may be added in future so callers should use named
+        arguments.
+
+        BzrLibraryState implements the Python 2.5 Context Manager protocol, and
+        can be used with the with statement. Upon __enter__ the global
+        variables in use by bzr are set, and they are cleared on __exit__.
+
+        :param setup_ui: If true (default) use a terminal UI; otherwise 
+            some other ui_factory must be assigned to `bzrlib.ui.ui_factory` by
+            the caller.
+        :param stdin, stdout, stderr: If provided, use these for terminal IO;
+            otherwise use the files in `sys`.
+        """
+        self.setup_ui = setup_ui
+        self.stdin = stdin
+        self.stdout = stdout
+        self.stderr = stderr
+
+    def __enter__(self):
+        # NB: This function tweaks so much global state it's hard to test it in
+        # isolation within the same interpreter.  It's not reached on normal
+        # in-process run_bzr calls.  If it's broken, we expect that
+        # TestRunBzrSubprocess may fail.
+        if version_info[3] == 'final':
+            from bzrlib.symbol_versioning import suppress_deprecation_warnings
+            suppress_deprecation_warnings(override=True)
+
+        import bzrlib.cleanup
+        import bzrlib.trace
+        self.cleanups = bzrlib.cleanup.ObjectWithCleanups()
+        bzrlib.trace.enable_default_logging()
+
+        if self.setup_ui:
+            import bzrlib.ui
+            stdin = self.stdin or sys.stdin
+            stdout = self.stdout or sys.stdout
+            stderr = self.stderr or sys.stderr
+            bzrlib.ui.ui_factory = bzrlib.ui.make_ui_for_terminal(
+                stdin, stdout, stderr)
+        global global_state
+        self.saved_state = global_state
+        global_state = self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        self.cleanups.cleanup_now()
+        import bzrlib.ui
+        bzrlib.trace._flush_stdout_stderr()
+        bzrlib.trace._flush_trace()
+        import bzrlib.osutils
+        bzrlib.osutils.report_extension_load_failures()
+        bzrlib.ui.ui_factory.__exit__(None, None, None)
+        bzrlib.ui.ui_factory = None
+        global global_state
+        global_state = self.saved_state
+        return False # propogate exceptions.
+
+
+def initialize(setup_ui=True, stdin=None, stdout=None, stderr=None):
     """Set up everything needed for normal use of bzrlib.
 
     Most applications that embed bzrlib, including bzr itself, should call
@@ -131,39 +227,20 @@
     More options may be added in future so callers should use named arguments.
 
     :param setup_ui: If true (default) use a terminal UI; otherwise 
-        something else must be put into `bzrlib.ui.ui_factory`.
+        some other ui_factory must be assigned to `bzrlib.ui.ui_factory` by
+        the caller.
     :param stdin, stdout, stderr: If provided, use these for terminal IO;
         otherwise use the files in `sys`.
+    :return: A context manager for the use of bzrlib. The __enter__ method of
+        this context needs to be called before it takes effect, and the __exit__
+        should be called by the caller before exiting their process or
+        otherwise stopping use of bzrlib. Advanced callers can use
+        BzrLibraryState directly.
     """
-    # TODO: mention this in a guide to embedding bzrlib
-    #
-    # NB: This function tweaks so much global state it's hard to test it in
-    # isolation within the same interpreter.  It's not reached on normal
-    # in-process run_bzr calls.  If it's broken, we expect that
-    # TestRunBzrSubprocess may fail.
-    
-    import atexit
-    import bzrlib.trace
-
-    bzrlib.trace.enable_default_logging()
-    atexit.register(bzrlib.trace._flush_stdout_stderr)
-    atexit.register(bzrlib.trace._flush_trace)
-
-    import bzrlib.ui
-    if stdin is None:
-        stdin = sys.stdin
-    if stdout is None:
-        stdout = sys.stdout
-    if stderr is None:
-        stderr = sys.stderr
-
-    if setup_ui:
-        bzrlib.ui.ui_factory = bzrlib.ui.make_ui_for_terminal(
-            stdin, stdout, stderr)
-
-    if bzrlib.version_info[3] == 'final':
-        from bzrlib.symbol_versioning import suppress_deprecation_warnings
-        suppress_deprecation_warnings(override=True)
-
-    import bzrlib.osutils
-    atexit.register(osutils.report_extension_load_failures)
+    return BzrLibraryState(setup_ui=setup_ui, stdin=stdin,
+        stdout=stdout, stderr=stderr)
+
+
+def test_suite():
+    import tests
+    return tests.test_suite()

=== 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 @@
 import sys
 import urllib
 
+import bzrlib
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
-import atexit
 import socket
 import thread
 import weakref
@@ -494,16 +494,16 @@
 class _DebugCounter(object):
     """An object that counts the HPSS calls made to each client medium.
 
-    When a medium is garbage-collected, or failing that when atexit functions
-    are run, the total number of calls made on that medium are reported via
-    trace.note.
+    When a medium is garbage-collected, or failing that when
+    bzrlib.global_state exits, the total number of calls made on that medium
+    are reported via trace.note.
     """
 
     def __init__(self):
         self.counts = weakref.WeakKeyDictionary()
         client._SmartClient.hooks.install_named_hook(
             'call', self.increment_call_count, 'hpss call counter')
-        atexit.register(self.flush_all)
+        bzrlib.global_state.cleanups.addCleanup(self.flush_all)
 
     def track(self, medium):
         """Start tracking calls made to a medium.

=== 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 @@
     _trace_file = old_trace_file
     bzr_logger = logging.getLogger('bzr')
     bzr_logger.removeHandler(new_handler)
-    # must be closed, otherwise logging will try to close it atexit, and the
+    # must be closed, otherwise logging will try to close it at exit, and the
     # file will likely already be closed underneath.
     new_handler.close()
     bzr_logger.handlers = old_handlers
@@ -540,7 +540,7 @@
 
 
 def _flush_stdout_stderr():
-    # installed into an atexit hook by bzrlib.initialize()
+    # called from the bzrlib library finalizer returned by bzrlib.initialize()
     try:
         sys.stdout.flush()
         sys.stderr.flush()
@@ -553,7 +553,7 @@
 
 
 def _flush_trace():
-    # run from atexit hook
+    # called from the bzrlib library finalizer returned by bzrlib.initialize()
     global _trace_file
     if _trace_file:
         _trace_file.flush()

=== 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 @@
     This tells the library how to display things to the user.  Through this
     layer different applications can choose the style of UI.
 
+    UI Factories are also context managers, for some syntactic sugar some users
+    need.
+
     :ivar suppressed_warnings: Identifiers for user warnings that should 
         no be emitted.
     """
@@ -123,6 +126,22 @@
         self.suppressed_warnings = set()
         self._quiet = False
 
+    def __enter__(self):
+        """Context manager entry support.
+
+        Override in a concrete factory class if initialisation before use is
+        needed.
+        """
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        """Context manager exit support.
+
+        Override in a concrete factory class if more cleanup than a simple
+        self.clear_term() is needed when the UIFactory is finished with.
+        """
+        self.clear_term()
+        return False # propogate exceptions.
+
     def be_quiet(self, state):
         """Tell the UI to be more quiet, or not.
 
@@ -352,7 +371,6 @@
                 "without an upgrade path.\n" % (inter.target._format,))
 
 
-
 class SilentUIFactory(UIFactory):
     """A UI Factory which never prints anything.
 

=== 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 @@
 to the Bazaar mailing list.  
 
 
+Using bzrlib
+############
+
+Within bzr
+==========
+
+When using bzrlib within the ``bzr`` program (for instance as a bzr
+plugin), bzrlib's global state is already available for use.
+
+From outside bzr
+================
+
+To use bzrlib outside of ``bzr`` some global state needs to be setup.
+bzrlib needs ways to handle user input, passwords, a place to emit
+progress bars, logging setup appropriately for your program. The easiest
+way to set all this up in the same fashion ``bzr`` does is to call
+``bzrlib.initialize``. This returns a context manager within which bzrlib
+functions will work correctly. See the pydoc for ``bzrlib.initialize`` for
+more information. In Python 2.4 the ``with`` keyword is not supported and
+so you need to use the context manager manually::
+
+  # This sets up your ~/.bzr.log, ui factory and so on and so forth. It is
+  # not safe to use as a doctest.
+  library_state = bzrlib.initialize()
+  library_state.__enter__()
+  try:
+      pass
+      # do stuff here
+  finally:
+      library_state.__exit__(None, None, None)
+
+
 Core classes
 ############