Merge lp:~mbp/bzr/499637-default-uifactory into lp:bzr

[Martin Pool]

[Martin Pool (mbp)]

This fixes https://bugs.edge.launchpad.net/bzr/+bug/499757 by making SilentUIFactory accept make_output_stream

[Martin Pool (mbp)]

Despite the name I haven't fixed bug 449637 here yet, because it causes a somewhat annoying circular import situation to try to set bzrlib.ui.ui_factory to an instance of bzrlib.ui.text.TextUIFactory...

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Despite the name I haven't fixed bug 449637 here yet, because it causes a somewhat annoying circular import situation to try to set bzrlib.ui.ui_factory to an instance of bzrlib.ui.text.TextUIFactory...
>

The bug states that the problem is that SilentUIFactory gives a
make_output_stream error. Is there something else going on that I don't
understand?

main() is the helper that sets the ui factory and trace settings. I
think it is reasonable that if someone wants to call 'run_bzr' directly,
they can set things up for themselves. (Otherwise they would call
main(), right?)

Anyway, your change is ok. I think it *does* break people who would be
using run_bzr() directly (without setting a ui_factory). Think of stuff
like run_bzr('diff'). It used to output content to sys.stdout even
without the ui_factory set. However, I can understand that Silent seems
to indicate that there wouldn't be any output.

Though if they are already running things directly, they also have to do
stuff like "install_bzr_command_hooks()". I did test that in bzr/2.0
doing this works and produces output:

from bzrlib import commands
commands.install_bzr_command_hooks()
commands.run_bzr(['diff', '-c', '-1'])

However on bzr.dev it produces a "make_output_stream not supported"
error. With your patch it succeeds, but produces no output.

Not failing is better than failing on every command (even ones that
don't produce output, or don't produce it through make_output_stream()).
I'd be slightly happier if we continued to produce output, but only a bit.

John
=:->
  review: approve
  merge: approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktPL/MACgkQJdeBCYSNAAPqagCeMAgFgmgHks7GacaYIYpdUqdH
tC0AoJZNkA1X6LkcQYVEKQQRB0IUEF7e
=qGMk
-----END PGP SIGNATURE-----

review: Approve

[Martin Pool (mbp)]

2010/1/15 John Arbash Meinel <email address hidden>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Martin Pool wrote:
>> Despite the name I haven't fixed bug 449637 here yet, because it causes a somewhat annoying circular import situation to try to set bzrlib.ui.ui_factory to an instance of bzrlib.ui.text.TextUIFactory...
>>
>
> The bug states that the problem is that SilentUIFactory gives a
> make_output_stream error. Is there something else going on that I don't
> understand?

There are two bugs here:

1- silentuifactory.make_output_stream errors - it would be more
reasonable for it to discard the output
2- the default for bzrlib should probably be text output to
stdout/stderr, as it once was, not silence

#2 was once mostly working because bulk output didn't go through the
uifactory. However, other things that did go through the uifactory
didn't work by default in the way you would have expected. So what's
changed there is just that the scope of the uifactory has grown so the
consequences of not setting one are larger.

This fixes #1; I'd like to fix the second too but it's a bit annoying
to get something from a submodule to initialize a variable in a
supermodule at load time because of circular import restrictions.

> main() is the helper that sets the ui factory and trace settings. I
> think it is reasonable that if someone wants to call 'run_bzr' directly,
> they can set things up for themselves. (Otherwise they would call
> main(), right?)

I think calling main() directly has always worked. The other case is
people just using arbitrary internal bzrlib functions.

> Anyway, your change is ok. I think it *does* break people who would be
> using run_bzr() directly (without setting a ui_factory). Think of stuff
> like run_bzr('diff'). It used to output content to sys.stdout even
> without the ui_factory set. However, I can understand that Silent seems
> to indicate that there wouldn't be any output.
>
> Though if they are already running things directly, they also have to do
> stuff like "install_bzr_command_hooks()". I did test that in bzr/2.0
> doing this works and produces output:
>
> from bzrlib import commands
> commands.install_bzr_command_hooks()
> commands.run_bzr(['diff', '-c', '-1'])
>
> However on bzr.dev it produces a "make_output_stream not supported"
> error. With your patch it succeeds, but produces no output.

So if I don't fix #2 above, they will need to call

make_ui_factory_for_terminal(sys.stdin, ...)

which is reasonable but I think not ideal.

> Not failing is better than failing on every command (even ones that
> don't produce output, or don't produce it through make_output_stream()).
> I'd be slightly happier if we continued to produce output, but only a bit.

--
Martin <http://launchpad.net/~mbp/>

[Martin Pool (mbp)]

https://bugs.edge.launchpad.net/bzr/+bug/507710 suggests just giving plugins a bzrlib.initialize() function.

=== modified file 'NEWS'
--- NEWS	2010-01-14 00:33:43 +0000
+++ NEWS	2010-01-14 08:45:22 +0000
@@ -90,6 +90,11 @@
 * Listen to the SIGWINCH signal to update the terminal width.
   (Vincent Ladeuil, #316357)
 
+* ``SilentUIFactory`` now supports ``make_output_stream`` and discards
+  whatever is written to it.  This un-breaks some plugin tests that
+  depended on this behaviour.
+  (Martin Pool, #499757)
+
 * The 2a format wasn't properly restarting autopacks when something
   changed underneath it (like another autopack). Now concurrent
   autopackers will properly succeed. (John Arbash Meinel, #495000)

=== modified file 'bzrlib/tests/per_uifactory/__init__.py'
--- bzrlib/tests/per_uifactory/__init__.py	2010-01-05 15:52:28 +0000
+++ bzrlib/tests/per_uifactory/__init__.py	2010-01-14 08:45:22 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2009 Canonical Ltd
+# Copyright (C) 2009-2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -76,12 +76,9 @@
         self._check_show_warning(msg)
 
     def test_make_output_stream(self):
-        # at the moment this is only implemented on text uis; i'm not sure
-        # what it should do elsewhere
-        try:
-            output_stream = self.factory.make_output_stream()
-        except NotImplementedError, e:
-            raise tests.TestSkipped(str(e))
+        # All UIs must now be able to at least accept output, even if they
+        # just discard it.
+        output_stream = self.factory.make_output_stream()
         output_stream.write('hello!')
 
     def test_transport_activity(self):

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2009-12-18 16:39:21 +0000
+++ bzrlib/ui/__init__.py	2010-01-14 08:45:22 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd
+# Copyright (C) 2005-2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -296,6 +296,9 @@
     def get_username(self, prompt, **kwargs):
         return None
 
+    def _make_output_stream_explicit(self, encoding, encoding_type):
+        return NullOutputStream(encoding)
+
     def show_error(self, msg):
         pass
 
@@ -361,3 +364,19 @@
 
     def log_transport_activity(self, display=False):
         pass
+
+
+class NullOutputStream(object):
+    """Acts like a file, but discard all output."""
+
+    def __init__(self, encoding):
+        self.encoding = encoding
+
+    def write(self, data):
+        pass
+
+    def writelines(self, data):
+        pass
+
+    def close(self):
+        pass