Bazaar

Merge lp:~jameinel/bzr/2.0.4-trace-failure into lp:bzr/2.0

  1. 2.0.4-trace-failure
  2. Merge into 2.0
Proposed by John A Meinel
Status: Merged
Approved by: Andrew Bennetts
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~jameinel/bzr/2.0.4-trace-failure
Merge into: lp:bzr/2.0
Diff against target: 67 lines (+27/-1)
3 files modified
NEWS (+6/-0)
bzrlib/tests/test_trace.py (+15/-0)
bzrlib/trace.py (+6/-1)
To merge this branch: bzr merge lp:~jameinel/bzr/2.0.4-trace-failure
Reviewer Review Type Date Requested Status
Andrew Bennetts Approve
Review via email: mp+16917@code.launchpad.net
To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

This fixes a fairly long-standing bug. If we failed to open ~/.bzr.log on the server, you end up getting the cryptic:
 No handlers could be found for logger "bzr"

This is because _open_bzr_log was trying to use the logging module to report the failure, but by that point in time we are *setting up* logging.

So I changed it to just report to stderr directly, which makes a lot more sense. It also means you'll get the path of the file that cannot be opened, which should make it clearer that it is happening server side, and not locally. (It won't always be 100% clear, but adding socket.gethostname() to that error message seemed a bit overkill.)

Revision history for this message
Andrew Bennetts (spiv) wrote :

No complaints from me!

review: Approve
Revision history for this message
Andrew Bennetts (spiv) wrote :

(I just sent this to PQM, btw.)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-01-05 00:01:54 +0000
3+++ NEWS 2010-01-06 17:50:24 +0000
4@@ -38,6 +38,12 @@
5 * Give a clearer message if the lockdir disappears after being apparently
6 successfully taken. (Martin Pool, #498378)
7
8+* If we fail to open ``~/.bzr.log`` write a clear message to stderr rather
9+ than using ``warning()``. The log file is opened before logging is set
10+ up, and it leads to very confusing: 'no handlers for "bzr"' messages for
11+ users, rather than something nicer.
12+ (John Arbash Meinel, Barry Warsaw, #503886)
13+
14 * The 2a format wasn't properly restarting autopacks when something
15 changed underneath it (like another autopack). Now concurrent
16 autopackers will properly succeed. (John Arbash Meinel, #495000)
17
18=== modified file 'bzrlib/tests/test_trace.py'
19--- bzrlib/tests/test_trace.py 2009-09-03 02:59:56 +0000
20+++ bzrlib/tests/test_trace.py 2010-01-06 17:50:24 +0000
21@@ -27,6 +27,7 @@
22
23 from bzrlib import (
24 errors,
25+ trace,
26 )
27 from bzrlib.tests import TestCaseInTempDir, TestCase
28 from bzrlib.trace import (
29@@ -238,6 +239,20 @@
30 tmp1.close()
31 tmp2.close()
32
33+ def test__open_bzr_log_uses_stderr_for_failures(self):
34+ # If _open_bzr_log cannot open the file, then we should write the
35+ # warning to stderr. Since this is normally happening before logging is
36+ # set up.
37+ self.addCleanup(setattr, sys, 'stderr', sys.stderr)
38+ self.addCleanup(setattr, trace, '_bzr_log_filename',
39+ trace._bzr_log_filename)
40+ sys.stderr = StringIO()
41+ # Set the log file to something that cannot exist
42+ os.environ['BZR_LOG'] = os.getcwd() + '/no-dir/bzr.log'
43+ logf = trace._open_bzr_log()
44+ self.assertIs(None, logf)
45+ self.assertContainsRe(sys.stderr.getvalue(),
46+ 'failed to open trace file: .*/no-dir/bzr.log')
47
48 class TestVerbosityLevel(TestCase):
49
50
51=== modified file 'bzrlib/trace.py'
52--- bzrlib/trace.py 2009-09-03 02:59:56 +0000
53+++ bzrlib/trace.py 2010-01-06 17:50:24 +0000
54@@ -237,7 +237,12 @@
55 bzr_log_file.write("bug reports to https://bugs.launchpad.net/bzr/+filebug\n\n")
56 return bzr_log_file
57 except IOError, e:
58- warning("failed to open trace file: %s" % (e))
59+ # If we are failing to open the log, then most likely logging has not
60+ # been set up yet. So we just write to stderr rather than using
61+ # 'warning()'. If we using warning(), users get the unhelpful 'no
62+ # handlers registered for "bzr"' when something goes wrong on the
63+ # server. (bug #503886)
64+ sys.stderr.write("failed to open trace file: %s\n" % (e,))
65 # TODO: What should happen if we fail to open the trace file? Maybe the
66 # objects should be pointed at /dev/null or the equivalent? Currently
67 # returns None which will cause failures later.

Subscribers

People subscribed via source and target branches