Opened 4 years ago

Closed 4 years ago

#6087 release blocker: regression closed fixed (fixed)

Restore (and test more reliabley) twisted.python.log's capture of warnings

Reported by: Itamar Turner-Trauring Owned by: Itamar Turner-Trauring
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Jean-Paul Calderone, ralphm Branch: branches/warnings-6087
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst

Description

Warnings are supposed to end up in the Twisted log once logging is initialized, but #5885 broke that functionality. It should be fixed (reverting #5885 would be somewhat painful at this point) and the tests improved so this doesn't happen again.

Change History (7)

comment:1 Changed 4 years ago by itamarst

Author: itamarst
Branch: branches/warnings-6087

(In [36022]) Branching to 'warnings-6087'

comment:2 Changed 4 years ago by Itamar Turner-Trauring

Cc: Jean-Paul Calderone ralphm added
Keywords: review added
Owner: Itamar Turner-Trauring deleted

I've fixed it, I think. Note that this involves a minor change in the way we do things; when we write a warning to a file in LogPublisher.showwarning, we use the *original* warnings.showwarning (or at least, what was there at import time). Previously we used the version captured by startLoggingWithObserver, which caused problems using LogPublisher.showwarning before logging had been initialized. It is vaguely possible this will break some obscure code somewhere that overrides showwarning sufficiently to actually write to files differently, and does the override in between importing twisted.python.log and starting twisted logging. I am inclined not to care, but I could come up with a different solution.

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/warnings-6087

comment:3 Changed 4 years ago by Itamar Turner-Trauring

Type: defectregression

comment:4 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Itamar Turner-Trauring

Thanks.

  1. in test_publisherWarningInitialization
    1. the builtin setattr is preferable to using warnings.__setattr__.
  2. twisted/python/log.py
    1. As discussed on IRC, the showwarning APIs here aren't intended to be used by applications. I think we should move towards deprecating them and not worry about all the super obscure edge cases in their behavior (related to what order events happen in, etc).

Not sure many more review comments make sense, given that hopefully the last point above means things will change significantly and for the better.

comment:5 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/warnings-6087 has started.

I implemented a simpler "fix"; we're back to the previous behavior from before the Python 3 changes, I believe, with some tests skipped or munged until they work.

comment:6 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring

Okay. Please merge.

comment:7 Changed 4 years ago by itamarst

Resolution: fixed
Status: newclosed

(In [36349]) Merge warnings-6087.

Author: itamar Review: exarkun Fixes: #6087

The Python 3 port of logging mistakenly broke warning integration; this fixes it. It's a .misc because from point of view of upgrading user nothing will have changed.

Note: See TracTickets for help on using tickets.