Merge lp:~mbp/bzr/417881-selftest-no-apport into lp:bzr

Proposed by Martin Pool on 2010-02-03
Status: Merged
Approved by: John A Meinel on 2010-02-03
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~mbp/bzr/417881-selftest-no-apport
Merge into: lp:bzr
Diff against target: 31 lines (+10/-0)
2 files modified
NEWS (+3/-0)
bzrlib/builtins.py (+7/-0)
To merge this branch: bzr merge lp:~mbp/bzr/417881-selftest-no-apport
Reviewer Review Type Date Requested Status
John A Meinel 2010-02-03 Approve on 2010-02-03
Review via email: mp+18527@code.launchpad.net

This proposal supersedes a proposal from 2010-02-02.

To post a comment you must log in.
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

Turn off apport during selftest, and put the traceback at the end where it's more useful to developers and less likely to be scrolled off by plugins.

Fixes bug 417881

John A Meinel (jameinel) wrote :

I think the specific layout can get a bit into bikeshedding, but if you like the look of this layout, then lets go with it.

review: Approve
Martin Pool (mbp) wrote :

I think I'll land this without the traceback changes and we can consider that separately.

Robert Collins (lifeless) wrote :

So I realise this has landed already, but I think its the wrong
approach ;)

Specifically, we should IMO disable apport *anyway* for development
trees - where there is a .bzr dir. That way, for selftest:
 - as packaged, we get apport bug reports about selftest (good)
 - as a dev environment, we don't get apport in our way (good)

-Rob

Martin Pool (mbp) wrote :

On 3 February 2010 17:18, Robert Collins <email address hidden> wrote:
> So I realise this has landed already, but I think its the wrong
> approach ;)
>
> Specifically, we should IMO disable apport *anyway* for development
> trees - where there is a .bzr dir. That way, for selftest:
>  - as packaged, we get apport bug reports about selftest (good)
>  - as a dev environment, we don't get apport in our way (good)

It's easy to change. Where do you think we should specify that
something is a development environment?

I don't think "not from a package" or "running from home" is perfect
because there are users who regularly run that way. But perhaps it is
the best tradeoff.

Alternatively we can set "debug_flags = no_apport" ourselves and
simply reverse this patch.

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

Robert Collins (lifeless) wrote :

On Wed, 2010-02-03 at 17:42 +0000, Martin Pool wrote:
> On 3 February 2010 17:18, Robert Collins <email address hidden> wrote:
> > So I realise this has landed already, but I think its the wrong
> > approach ;)
> >
> > Specifically, we should IMO disable apport *anyway* for development
> > trees - where there is a .bzr dir. That way, for selftest:
> > - as packaged, we get apport bug reports about selftest (good)
> > - as a dev environment, we don't get apport in our way (good)
>
> It's easy to change. Where do you think we should specify that
> something is a development environment?

I was thinking if we're running from source is a good heuristic - users
that run from source and aren't up to reporting good bugs are a small
portion of our userbase, I think.

> I don't think "not from a package" or "running from home" is perfect
> because there are users who regularly run that way. But perhaps it is
> the best tradeoff.
>
> Alternatively we can set "debug_flags = no_apport" ourselves and
> simply reverse this patch.

Or we could do that instead/too. Mainly I'm saying that a user on Ubuntu
with a packaged bzr that breaks loading tests should generate an apport
problem report.

-Rob

Martin Pool (mbp) wrote :

So shall we perhaps pull this out and just tell developers to set -Dno_apport?

Robert Collins (lifeless) wrote :

On Wed, 2010-02-03 at 18:21 +0000, Martin Pool wrote:
> So shall we perhaps pull this out and just tell developers to set -Dno_apport?

Yes, I think so. Perhaps a note in HACKING noting this and suggesting a
bazaar.conf stanza for it?

-Rob

Andrew Bennetts (spiv) wrote :

Martin Pool wrote:
> So shall we perhaps pull this out and just tell developers to set -Dno_apport?

FWIW, this developer already set -Dno_apport several weeks ago, so that plan
gets +1 from me :)

-Andrew.

Ian Clatworthy (ian-clatworthy) wrote :

Andrew Bennetts wrote:
> Martin Pool wrote:
>> So shall we perhaps pull this out and just tell developers to set -Dno_apport?
>
> FWIW, this developer already set -Dno_apport several weeks ago, so that plan
> gets +1 from me :)

Me too.

Ian C.

Martin Pool (mbp) wrote :

reversion landed

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-02-03 10:06:19 +0000
3+++ NEWS 2010-02-03 15:45:25 +0000
4@@ -28,6 +28,9 @@
5 Testing
6 *******
7
8+* Don't use Apport for errors while loading or running tests.
9+ (Martin Pool, #417881)
10+
11 * Stop sending apport crash files to ``.cache`` in the directory from
12 which ``bzr selftest`` was run. (Martin Pool, #422350)
13
14
15=== modified file 'bzrlib/builtins.py'
16--- bzrlib/builtins.py 2010-01-29 10:36:23 +0000
17+++ bzrlib/builtins.py 2010-02-03 15:45:25 +0000
18@@ -3524,6 +3524,13 @@
19
20 # Make deprecation warnings visible, unless -Werror is set
21 symbol_versioning.activate_deprecation_warnings(override=False)
22+
23+ # Whatever you normally want, for the purposes of running selftest you
24+ # probably actually want to see the exception, not to turn it into an
25+ # apport failure. This is specifically turned off again for tests of
26+ # apport. We turn it off here so that eg a SyntaxError loading the
27+ # tests is caught.
28+ os.environ['APPORT_DISABLE'] = '1'
29
30 if cache_dir is not None:
31 tree_creator.TreeCreator.CACHE_ROOT = osutils.abspath(cache_dir)