1:20 p.m.
(Sorry about the length of this email)
Python 2.7 deprecated the PyCObject API in favor of a new "capsule" API.
http://docs.python.org/dev/whatsnew/2.7.html#capsules
The deprecations are set to "ignore" by default, so in theory the API
still works: every time an extension uses the API, a deprecation warning
is emitted, but then "swallowed" by the filters, and the call succeeds.
However, if someone overrides the process-wide warnings settings, then
the API can fail altogether, raising a PendingDeprecationWarning
exception (which in CPython terms means setting a thread-specific error
state and returning NULL).
There are at least 15 extension modules that use this API in Fedora 14,
and most of the C code I've seen that uses this API doesn't expect it to
return NULL. This can lead to hard failures in which /usr/bin/python
aborts with an assertion failure (or even a segfault).
This has caused at least one app to fail (virt-manager, see bug 620216,
due to it modifying the warning settings: fixed), so I've been
doublechecking the scope of usage of the PyCObject API, and I've filed
bugs against components that are possibly affected:
https://bugzilla.redhat.com/showdependencytree.cgi?id=620842&hide_res...
This was on a test machine, so there may be some I've missed.
Unfortunately, the list of affected modules includes pygtk2 and
pygobject, numpy, selinux, and SWIG.
To add to the "fun", the pygtk2/pygobject modules expose the API via
macros in a public header file, so that to fix this we'd have to both
rebuild those modules, and rebuild users of the header file.
You can typically trigger a hard failure of the API via:
>> import warnings
>> warnings.filterwarnings('error')
and then try to import one of the
affected modules. I've tried to give
exact reproducers where I have them in each of the bugs.
But if nothing touches the warning settings, you'll never see a problem.
Possible ways forward:
(a) don't fix this; treat enabling the warning in the "Doctor, it
hurts when I do this! So don't do that!" category, and add this to the
release notes. Patch Python code that enables the warning so that it
doesn't.
(b) try to fix the ones that are self-contained; send fixes upstream
(c) try to fix them all; send fixes upstream
(d) hack the python rpm to remove this warning; this would be a
significant change from upstream, given that it's already disabled.
One issue here is that this API expresses a binary interface between
different Python modules, and that we don't yet have a way to express
this at the rpm metadata level. I think we should, to make it easier to
track these issues in the future. I don't think it's possible to track
these automatically, but we could do it manually.
Suggested way to express this:
Modules that provide a capsule named
"full.dotted.path.to.module.CAPSULENAME"
as passed to PyCapsule_Import [1] should have this in their specfile
Provides: PyCapsule(full.dotted.path.to.module.CAPSULENAME)
for the subpackage holding the module in its %files manifest.
Modules that import a capsule should have a corresponding:
Requires: PyCapsule(full.dotted.path.to.module.CAPSULENAME)
in the appropriate subpackage.
So, for example, if we apply the patch I've proposed upstream for pygtk,
then pygtk2 should have a:
Provides: PyCapsule(gtk._gtk._PyGtk_API)
and anything using it needs a:
Requires: PyCapsule(gtk._gtk._PyGtk_API)
This wouldn't solve all the problems: we'd also need the legacy users of
the macro to be rebuilt, and upgrading pygtk2 without upgrading them
would lead to a runtime failure when the PyCObject_Ptr call fails (we
could potentially supply both hooks, though this would fail if ignoring
deprecation warnings was disabled).
So upon switching from PyCObject to the capsule API and adding the:
Provides above, pygtk2 would also need to add a Conflicts on each of the
known users of the API, <= the last version (known to use the broken
API); these would then need to be rebuilt, and rpm/yum would have enough
information to enforce valid combinations.
(None of this seems to address the issue of ABI changes between
different supposedly-compatible versions of the API; perhaps we need to
treat these capsule names like SONAMEs, and have a numbering within
them?)
Personally, I'm leaning towards option (a) above (the "don't override
warnings" option): closing the various as WONTFIX, and adding a section
to the release notes, whilst working towards fixing this in Fedora 15.
Affected applications should be patched in Fedora 14 to avoid touching
the relevant warning setting, and we'll fix the root cause in Fedora 15.
Thoughts?
Dave
[1] http://docs.python.org/dev/c-api/capsule.html#PyCapsule_Import
[2] https://bugzilla.gnome.org/show_bug.cgi?id=623965
4:44 p.m.
On Fri, Aug 13, 2010 at 10:20 AM, David Malcolm <dmalcolm(a)redhat.com> wrote:
Personally, I'm leaning towards option (a) above (the
"don't override
warnings" option): closing the various as WONTFIX, and adding a section
to the release notes, whilst working towards fixing this in Fedora 15.
Affected applications should be patched in Fedora 14 to avoid touching
the relevant warning setting, and we'll fix the root cause in Fedora 15.
Thoughts?
Dave
I'll have to read up closely on my upstream numpy and scipy are doing
in their development branches with regard to this. I think for F14
I'm going to have to live with using the CObject API for these. I
think the next major version of numpy is going to have this mostly
sorted but i'm not keen on pushing it into F14..not unless I have to.
A guarantee you there is a crapload of homebrew academic/science code
out there that uses the CObject API in a numpy oriented workflow. You
are definitely going to need a release note blurb with a reference
with more detail information on how to work around the problem for
people who run into it with their home brew code.
-jef"not cool"spaleta
7:24 p.m.
On Fri, 2010-08-13 at 13:44 -0800, Jeff Spaleta wrote:
On Fri, Aug 13, 2010 at 10:20 AM, David Malcolm
<dmalcolm(a)redhat.com> wrote:
> Personally, I'm leaning towards option (a) above (the "don't override
> warnings" option): closing the various as WONTFIX, and adding a section
> to the release notes, whilst working towards fixing this in Fedora 15.
> Affected applications should be patched in Fedora 14 to avoid touching
> the relevant warning setting, and we'll fix the root cause in Fedora 15.
>
> Thoughts?
> Dave
I'll have to read up closely on my upstream numpy and scipy are doing
in their development branches with regard to this. I think for F14
I'm going to have to live with using the CObject API for these. I
think the next major version of numpy is going to have this mostly
sorted but i'm not keen on pushing it into F14..not unless I have to.
A guarantee you there is a crapload of homebrew academic/science code
out there that uses the CObject API in a numpy oriented workflow. You
are definitely going to need a release note blurb with a reference
with more detail information on how to work around the problem for
people who run into it with their home brew code.
Agreed.
-jef"not cool"spaleta
Sorry about this.
7:49 p.m.
On Fri, Aug 13, 2010 at 4:24 PM, David Malcolm <dmalcolm(a)redhat.com> wrote:
Sorry about this.
You don't know how sorry! You've made it onto my Christmas Card list.
Which means I send you a live puppy in the mail COD overnight delivery
for Christmas day to your place of work. Now you have a choice. You
can either be at work on Christmas Day waiting with cash in hand to
free that poor poor puppy from its incarceration.. or you don't.
-jef"only 95% evil"spaleta
6:38 p.m.
On Fri, Aug 13, 2010 at 02:20:51PM -0400, David Malcolm wrote:
(Sorry about the length of this email)
Python 2.7 deprecated the PyCObject API in favor of a new "capsule" API.
http://docs.python.org/dev/whatsnew/2.7.html#capsules
The deprecations are set to "ignore" by default, so in theory the API
still works: every time an extension uses the API, a deprecation warning
is emitted, but then "swallowed" by the filters, and the call succeeds.
However, if someone overrides the process-wide warnings settings, then
the API can fail altogether, raising a PendingDeprecationWarning
exception (which in CPython terms means setting a thread-specific error
state and returning NULL).
Do I understand correctly that when raising this in python code, it's just
a warning but when raising this in CPython, it's the same as an exception
and thus causes problems?
There are at least 15 extension modules that use this API in Fedora
14,
and most of the C code I've seen that uses this API doesn't expect it to
return NULL. This can lead to hard failures in which /usr/bin/python
aborts with an assertion failure (or even a segfault).
This has caused at least one app to fail (virt-manager, see bug 620216,
due to it modifying the warning settings: fixed), so I've been
doublechecking the scope of usage of the PyCObject API, and I've filed
bugs against components that are possibly affected:
https://bugzilla.redhat.com/showdependencytree.cgi?id=620842&hide_res...
This was on a test machine, so there may be some I've missed.
Unfortunately, the list of affected modules includes pygtk2 and
pygobject, numpy, selinux, and SWIG.
To add to the "fun", the pygtk2/pygobject modules expose the API via
macros in a public header file, so that to fix this we'd have to both
rebuild those modules, and rebuild users of the header file.
You can typically trigger a hard failure of the API via:
>>> import warnings
>>> warnings.filterwarnings('error')
and then try to import one of the affected modules. I've tried to give
exact reproducers where I have them in each of the bugs.
But if nothing touches the warning settings, you'll never see a problem.
What about warnings.filterwarnings('default') (or 'always',
'module', or
'once')?
Does this also reliably trigger the hard failure?
PYTHONWARNINGS='error' python PROGRAM
Possible ways forward:
(a) don't fix this; treat enabling the warning in the "Doctor, it
hurts when I do this! So don't do that!" category, and add this to the
release notes. Patch Python code that enables the warning so that it
doesn't.
(b) try to fix the ones that are self-contained; send fixes upstream
(c) try to fix them all; send fixes upstream
(d) hack the python rpm to remove this warning; this would be a
significant change from upstream, given that it's already disabled.
Taking the next bit out of order:
Personally, I'm leaning towards option (a) above (the
"don't override
warnings" option): closing the various as WONTFIX, and adding a section
to the release notes, whilst working towards fixing this in Fedora 15.
Affected applications should be patched in Fedora 14 to avoid touching
the relevant warning setting, and we'll fix the root cause in Fedora 15.
Is it overriding the warnings option that causes a problem or is it *only*
setting the warnings filter to 'error' that is the problem? I think that
setting the warning level to always, default, module, or once should be
supported. Setting a "warning" to "error" could be seen as
"buyer beware",
though. ie: if it's only error that's affected, then (a) seems okay. If
the others also cause issues, then I think (a) is the wrong fix.
One issue here is that this API expresses a binary interface between
different Python modules, and that we don't yet have a way to express
this at the rpm metadata level. I think we should, to make it easier to
track these issues in the future. I don't think it's possible to track
these automatically, but we could do it manually.
Tracking this manualy is no good unless you can explain to people how to
detect it. Once you can explain how to manually detect it, it might be
possible to automatically detect it....
Suggested way to express this:
Modules that provide a capsule named
"full.dotted.path.to.module.CAPSULENAME"
as passed to PyCapsule_Import [1] should have this in their specfile
Provides: PyCapsule(full.dotted.path.to.module.CAPSULENAME)
for the subpackage holding the module in its %files manifest.
Modules that import a capsule should have a corresponding:
Requires: PyCapsule(full.dotted.path.to.module.CAPSULENAME)
in the appropriate subpackage.
So, for example, if we apply the patch I've proposed upstream for pygtk,
then pygtk2 should have a:
Provides: PyCapsule(gtk._gtk._PyGtk_API)
What's the dotted path here represent? Is that a file on the filesystem?
Is it a function in a pygtk .c file? Is it a string that's exported by
pygtk via some PyCapsule function call?
and anything using it needs a:
Requires: PyCapsule(gtk._gtk._PyGtk_API)
What is meant by "using it"? Python code that does import gtk? Or
only
C code that calls some PyCapsule function to ask for that to be returned?
This wouldn't solve all the problems: we'd also need the
legacy users of
the macro to be rebuilt, and upgrading pygtk2 without upgrading them
would lead to a runtime failure when the PyCObject_Ptr call fails (we
could potentially supply both hooks, though this would fail if ignoring
deprecation warnings was disabled).
So upon switching from PyCObject to the capsule API and adding the:
Provides above, pygtk2 would also need to add a Conflicts on each of the
known users of the API, <= the last version (known to use the broken
API); these would then need to be rebuilt, and rpm/yum would have enough
information to enforce valid combinations.
This seems wrong but I could just be too used to thinking of things in terms
of SQL. You have a many to one relationship here and you're putting the
data in the record that is the one instead of the many.
Maybe someone else can offer a better model for doing this.
Also, it seems like this is something that we're fixing on the boundary
between releases where we traditionally don't treat the Provides/Requires
situation wrt the already installed packages quite the same.
(None of this seems to address the issue of ABI changes between
different supposedly-compatible versions of the API; perhaps we need to
treat these capsule names like SONAMEs, and have a numbering within
them?)
I'd love to see you get SONAMEs with API versions into upstream python for
extensions and pure python code :-) Not sure if something less than
using only upstream data is a good idea though.
-Toshio
7:24 p.m.
On Fri, 2010-08-13 at 19:38 -0400, Toshio Kuratomi wrote:
On Fri, Aug 13, 2010 at 02:20:51PM -0400, David Malcolm wrote:
> (Sorry about the length of this email)
>
> Python 2.7 deprecated the PyCObject API in favor of a new "capsule" API.
> http://docs.python.org/dev/whatsnew/2.7.html#capsules
>
> The deprecations are set to "ignore" by default, so in theory the API
> still works: every time an extension uses the API, a deprecation warning
> is emitted, but then "swallowed" by the filters, and the call succeeds.
>
> However, if someone overrides the process-wide warnings settings, then
> the API can fail altogether, raising a PendingDeprecationWarning
> exception (which in CPython terms means setting a thread-specific error
> state and returning NULL).
>
Do I understand correctly that when raising this in python code, it's just
a warning but when raising this in CPython, it's the same as an exception
and thus causes problems?
Not quite. With both, a warning is a potential exception. Essentially,
both pure python and C python code use warnings.warn() to potentially
emit warnings. The warnings module itself is implemented in a mix of
python and C (for speed). When something does warnings.warn(), the
particular warning is compared against warnings.filters, and this
determines what happens next: the warning can be ignored, or a message
can be printed to stderr, or an exception can be raised.
See http://docs.python.org/library/warnings.html
It's when an exception is raised that things go badly wrong: this is
implemented at the C level by returning NULL, and various modules aren't
written to cope with that.
> There are at least 15 extension modules that use this API in
Fedora 14,
> and most of the C code I've seen that uses this API doesn't expect it to
> return NULL. This can lead to hard failures in which /usr/bin/python
> aborts with an assertion failure (or even a segfault).
>
> This has caused at least one app to fail (virt-manager, see bug 620216,
> due to it modifying the warning settings: fixed), so I've been
> doublechecking the scope of usage of the PyCObject API, and I've filed
> bugs against components that are possibly affected:
> https://bugzilla.redhat.com/showdependencytree.cgi?id=620842&hide_res...
>
> This was on a test machine, so there may be some I've missed.
>
> Unfortunately, the list of affected modules includes pygtk2 and
> pygobject, numpy, selinux, and SWIG.
>
> To add to the "fun", the pygtk2/pygobject modules expose the API via
> macros in a public header file, so that to fix this we'd have to both
> rebuild those modules, and rebuild users of the header file.
>
>
> You can typically trigger a hard failure of the API via:
> >>> import warnings
> >>> warnings.filterwarnings('error')
> and then try to import one of the affected modules. I've tried to give
> exact reproducers where I have them in each of the bugs.
>
> But if nothing touches the warning settings, you'll never see a problem.
>
What about warnings.filterwarnings('default') (or 'always',
'module', or
'once')?
You can see the default warnings here:
>> import warnings
>> warnings.filters
[('ignore', None, <type
'exceptions.DeprecationWarning'>, None, 0),
('ignore', None, <type 'exceptions.PendingDeprecationWarning'>,
None,
0), ('ignore', None, <type 'exceptions.ImportWarning'>, None, 0),
('ignore', None, <type 'exceptions.BytesWarning'>, None, 0)]
i.e. "ignore the four listed exceptions"
Unfortunately, "warnings.resetwarnings" clears these filters, rather
than restoring them to their defaults:
>> warnings.resetwarnings()
>> warnings.filters
[]
Also, note that:
>> warnings.filterwarnings('default')
also
doesn't reset warnings to the default; it adds a rule to print to
stderr for all warnings to the front of the rules. This means that you
get a lot of noise on stderr, but everything should continue to work.
Does this also reliably trigger the hard failure?
PYTHONWARNINGS='error' python PROGRAM
Depends on the program (it could override things further), but in
general, yes:
$ PYTHONWARNINGS='error' python -c "import gtk"
python: /root/rpmbuild/BUILD/Python-2.7/Objects/dictobject.c:759:
PyDict_SetItem: Assertion `value' failed.
Aborted
> Possible ways forward:
> (a) don't fix this; treat enabling the warning in the "Doctor, it
> hurts when I do this! So don't do that!" category, and add this to the
> release notes. Patch Python code that enables the warning so that it
> doesn't.
> (b) try to fix the ones that are self-contained; send fixes upstream
> (c) try to fix them all; send fixes upstream
> (d) hack the python rpm to remove this warning; this would be a
> significant change from upstream, given that it's already disabled.
>
Taking the next bit out of order:
> Personally, I'm leaning towards option (a) above (the "don't override
> warnings" option): closing the various as WONTFIX, and adding a section
> to the release notes, whilst working towards fixing this in Fedora 15.
> Affected applications should be patched in Fedora 14 to avoid touching
> the relevant warning setting, and we'll fix the root cause in Fedora 15.
>
Is it overriding the warnings option that causes a problem or is it *only*
setting the warnings filter to 'error' that is the problem? I think that
setting the warning level to always, default, module, or once should be
supported. Setting a "warning" to "error" could be seen as
"buyer beware",
though. ie: if it's only error that's affected, then (a) seems okay. If
the others also cause issues, then I think (a) is the wrong fix.
If you set it to "always", "default", "module", or
"once" you'll get
noise on stderr, but it won't trigger the hard failure.
It's only on setting it to "error" that you get the hard failure.
> One issue here is that this API expresses a binary interface
between
> different Python modules, and that we don't yet have a way to express
> this at the rpm metadata level. I think we should, to make it easier to
> track these issues in the future. I don't think it's possible to track
> these automatically, but we could do it manually.
>
Tracking this manualy is no good unless you can explain to people how to
detect it. Once you can explain how to manually detect it, it might be
possible to automatically detect it....
You have to scrape through for ABI calls to PyCObject: the presence of
the calls are visible in the ELF metadata, but not the exact strings.
Actually, it _might_ be possible to figure them out via disassembly of
the machine code, but this seems fragile.
> Suggested way to express this:
> Modules that provide a capsule named
> "full.dotted.path.to.module.CAPSULENAME"
> as passed to PyCapsule_Import [1] should have this in their specfile
> Provides: PyCapsule(full.dotted.path.to.module.CAPSULENAME)
> for the subpackage holding the module in its %files manifest.
>
>
> Modules that import a capsule should have a corresponding:
> Requires: PyCapsule(full.dotted.path.to.module.CAPSULENAME)
> in the appropriate subpackage.
>
>
> So, for example, if we apply the patch I've proposed upstream for pygtk,
> then pygtk2 should have a:
> Provides: PyCapsule(gtk._gtk._PyGtk_API)
>
What's the dotted path here represent? Is that a file on the filesystem?
Is it a function in a pygtk .c file? Is it a string that's exported by
pygtk via some PyCapsule function call?
The latter.
> and anything using it needs a:
> Requires: PyCapsule(gtk._gtk._PyGtk_API)
>
What is meant by "using it"? Python code that does import gtk? Or only
C code that calls some PyCapsule function to ask for that to be returned?
The latter.
> This wouldn't solve all the problems: we'd also need the
legacy users of
> the macro to be rebuilt, and upgrading pygtk2 without upgrading them
> would lead to a runtime failure when the PyCObject_Ptr call fails (we
> could potentially supply both hooks, though this would fail if ignoring
> deprecation warnings was disabled).
>
> So upon switching from PyCObject to the capsule API and adding the:
> Provides above, pygtk2 would also need to add a Conflicts on each of the
> known users of the API, <= the last version (known to use the broken
> API); these would then need to be rebuilt, and rpm/yum would have enough
> information to enforce valid combinations.
>
This seems wrong but I could just be too used to thinking of things in terms
of SQL. You have a many to one relationship here and you're putting the
data in the record that is the one instead of the many.
The problem as I see it is
that the existing packages have the metadata
they do, and this can't be changed without using a time machine.
Maybe someone else can offer a better model for doing this.
Also, it seems like this is something that we're fixing on the boundary
between releases where we traditionally don't treat the Provides/Requires
situation wrt the already installed packages quite the same.
True
> (None of this seems to address the issue of ABI changes between
> different supposedly-compatible versions of the API; perhaps we need to
> treat these capsule names like SONAMEs, and have a numbering within
> them?)
>
I'd love to see you get SONAMEs with API versions into upstream python for
extensions and pure python code :-) Not sure if something less than
using only upstream data is a good idea though.
Noted.
Dave
8:57 p.m.
On Fri, Aug 13, 2010 at 08:24:07PM -0400, David Malcolm wrote:
On Fri, 2010-08-13 at 19:38 -0400, Toshio Kuratomi wrote:
> On Fri, Aug 13, 2010 at 02:20:51PM -0400, David Malcolm wrote:
> > Possible ways forward:
> > (a) don't fix this; treat enabling the warning in the "Doctor, it
> > hurts when I do this! So don't do that!" category, and add this to
the
> > release notes. Patch Python code that enables the warning so that it
> > doesn't.
> > (b) try to fix the ones that are self-contained; send fixes upstream
> > (c) try to fix them all; send fixes upstream
> > (d) hack the python rpm to remove this warning; this would be a
> > significant change from upstream, given that it's already disabled.
> >
> Taking the next bit out of order:
>
> > Personally, I'm leaning towards option (a) above (the "don't
override
> > warnings" option): closing the various as WONTFIX, and adding a section
> > to the release notes, whilst working towards fixing this in Fedora 15.
> > Affected applications should be patched in Fedora 14 to avoid touching
> > the relevant warning setting, and we'll fix the root cause in Fedora 15.
> >
> Is it overriding the warnings option that causes a problem or is it *only*
> setting the warnings filter to 'error' that is the problem? I think that
> setting the warning level to always, default, module, or once should be
> supported. Setting a "warning" to "error" could be seen as
"buyer beware",
> though. ie: if it's only error that's affected, then (a) seems okay. If
> the others also cause issues, then I think (a) is the wrong fix.
If you set it to "always", "default", "module", or
"once" you'll get
noise on stderr, but it won't trigger the hard failure.
It's only on setting it to "error" that you get the hard failure.
Okay -- so it sounds like:
* When used with pure python code, the warnings mechanism functions as documented
* When C code is involved *and* the warnings filter has been set to 'error'
(not when set to 'default', 'once', 'module', etc) then an
exception is
being raised where most C code is not expecting it.
* By not knowing to deal with that exception condition, the C code is
subject to abort or SegFault.
One further question: Does this only cause problems with the PendingDeprecationWarning?
ie: Can code do this without a problem?:
import warnings
warnings.simplefilter('error')
warnings.simplefilter('default', PendingDeprecationWarning)
If this is okay, then I'd modify your point (a) to be this plan:
When code that turns all warnings into errors is encountered, have it
instead cause PendingDeprecationWarning to print to stderr via
warnings.simplefilter('default', PendingDeprecationWarning)
Send that patch to upstream with the explanation that setting warnings to
error outside of testing is not good with python-2.7 for a couple years
because C code that uses the only recently deprecated old PyCObject API is
likely to segfault or abort when this is done.
Anyone who wants to can work on porting the PyCObject API calls to PyCapsule
but this is not a Fedora requirement. If we happen to notice it being used,
feel free to notify upstream about the dangers.
> > One issue here is that this API expresses a binary interface between
> > different Python modules, and that we don't yet have a way to express
> > this at the rpm metadata level. I think we should, to make it easier to
> > track these issues in the future. I don't think it's possible to
track
> > these automatically, but we could do it manually.
> >
> Tracking this manualy is no good unless you can explain to people how to
> detect it. Once you can explain how to manually detect it, it might be
> possible to automatically detect it....
You have to scrape through for ABI calls to PyCObject: the presence of
the calls are visible in the ELF metadata, but not the exact strings.
Actually, it _might_ be possible to figure them out via disassembly of
the machine code, but this seems fragile.
This already sounds like something that is too involved for maintainers and
package reviewers to do. I think this might be something that doesn't leave
the drawing board without tooling to at least do part of the detective work.
-TOshio
4:18 a.m.
Toshio Kuratomi wrote:
This already sounds like something that is too involved for
maintainers
and package reviewers to do. I think this might be something that doesn't
leave the drawing board without tooling to at least do part of the
detective work.
Well, people, as opposed to automated tools, will probably want to look at
the source code, not the machine code. (For automated tools, both approaches
have their pitfalls. This is a static analysis problem, those are always
hard to solve, the completely general case being entirely undecidable.)
Kevin Kofler
1:03 p.m.
On Fri, 2010-08-13 at 21:57 -0400, Toshio Kuratomi wrote:
On Fri, Aug 13, 2010 at 08:24:07PM -0400, David Malcolm wrote:
> On Fri, 2010-08-13 at 19:38 -0400, Toshio Kuratomi wrote:
> > On Fri, Aug 13, 2010 at 02:20:51PM -0400, David Malcolm wrote:
> > > Possible ways forward:
> > > (a) don't fix this; treat enabling the warning in the "Doctor,
it
> > > hurts when I do this! So don't do that!" category, and add this
to the
> > > release notes. Patch Python code that enables the warning so that it
> > > doesn't.
> > > (b) try to fix the ones that are self-contained; send fixes upstream
> > > (c) try to fix them all; send fixes upstream
> > > (d) hack the python rpm to remove this warning; this would be a
> > > significant change from upstream, given that it's already disabled.
> > >
> > Taking the next bit out of order:
> >
> > > Personally, I'm leaning towards option (a) above (the "don't
override
> > > warnings" option): closing the various as WONTFIX, and adding a
section
> > > to the release notes, whilst working towards fixing this in Fedora 15.
> > > Affected applications should be patched in Fedora 14 to avoid touching
> > > the relevant warning setting, and we'll fix the root cause in Fedora
15.
> > >
> > Is it overriding the warnings option that causes a problem or is it *only*
> > setting the warnings filter to 'error' that is the problem? I think
that
> > setting the warning level to always, default, module, or once should be
> > supported. Setting a "warning" to "error" could be seen as
"buyer beware",
> > though. ie: if it's only error that's affected, then (a) seems okay.
If
> > the others also cause issues, then I think (a) is the wrong fix.
>
> If you set it to "always", "default", "module", or
"once" you'll get
> noise on stderr, but it won't trigger the hard failure.
>
> It's only on setting it to "error" that you get the hard failure.
>
Okay -- so it sounds like:
* When used with pure python code, the warnings mechanism functions as documented
* When C code is involved *and* the warnings filter has been set to 'error'
(not when set to 'default', 'once', 'module', etc) then an
exception is
being raised where most C code is not expecting it.
I'm not sure I'd use
the word "most" here; but there are indeed a number
of significant code paths across Fedora where C code is not expecting it
(e.g. PyGTK initialization).
* By not knowing to deal with that exception condition, the C code
is
subject to abort or SegFault.
One further question: Does this only cause problems with the PendingDeprecationWarning?
ie: Can code do this without a problem?:
import warnings
warnings.simplefilter('error')
warnings.simplefilter('default', PendingDeprecationWarning)
This suppresses the crash, but leads to noise on stderr:
warnings.simplefilter adds the filter to the head of the list, so the
2nd line is run before the first.
If you change the 2nd line to "ignore", then things work without noise:
>> import warnings
>> warnings.simplefilter('error')
>> warnings.simplefilter('ignore', PendingDeprecationWarning)
>> import gtk
So I'm inclined to say: if you have code that enables warnings with
"error", please add "ignore" on PendingDeprecationWarning as well, to
avoid problems from these modules.
If this is okay, then I'd modify your point (a) to be this plan:
When code that turns all warnings into errors is encountered, have it
instead cause PendingDeprecationWarning to print to stderr via
warnings.simplefilter('default', PendingDeprecationWarning)
I don't like this approach: it's a divergence from upstream, and it
seems like magic: it seems to be second-guessing the API call.
It seems simpler and clearer to ask that code that enables errors on
warnings need to turn it off for PendingDeprecationWarning to be able to
use modules that are known to break in the presence of the
PendingDeprecationWarning-treated-as-exceptions.
Send that patch to upstream with the explanation that setting
warnings to
error outside of testing is not good with python-2.7 for a couple years
because C code that uses the only recently deprecated old PyCObject API is
likely to segfault or abort when this is done.
Anyone who wants to can work on porting the PyCObject API calls to PyCapsule
but this is not a Fedora requirement. If we happen to notice it being used,
feel free to notify upstream about the dangers.
I've done this for PyGTK.
> > > One issue here is that this API expresses a binary
interface between
> > > different Python modules, and that we don't yet have a way to express
> > > this at the rpm metadata level. I think we should, to make it easier to
> > > track these issues in the future. I don't think it's possible to
track
> > > these automatically, but we could do it manually.
> > >
> > Tracking this manualy is no good unless you can explain to people how to
> > detect it. Once you can explain how to manually detect it, it might be
> > possible to automatically detect it....
>
> You have to scrape through for ABI calls to PyCObject: the presence of
> the calls are visible in the ELF metadata, but not the exact strings.
> Actually, it _might_ be possible to figure them out via disassembly of
> the machine code, but this seems fragile.
>
This already sounds like something that is too involved for maintainers and
package reviewers to do. I think this might be something that doesn't leave
the drawing board without tooling to at least do part of the detective work.
Fundamentally the review process for this involves grepping through the
source code; any usage of a PyCObject_* function will return NULL if
PendingDeprectationWarning is set to "error". It needs a little ability
to read C code, but (I hope) not much. The issue with macros in the
header files is a nasty gotcha, though :-(
I had a go at writing some release notes for this issue here:
https://fedoraproject.org/wiki/Features/Python_2.7#Caveat:_PyCObject_and_...
How does this look?
Dave
2:14 p.m.
On Mon, Aug 16, 2010 at 02:03:51PM -0400, David Malcolm wrote:
On Fri, 2010-08-13 at 21:57 -0400, Toshio Kuratomi wrote:
> > > On Fri, Aug 13, 2010 at 02:20:51PM -0400, David Malcolm wrote:
> If this is okay, then I'd modify your point (a) to be this plan:
>
> When code that turns all warnings into errors is encountered, have it
> instead cause PendingDeprecationWarning to print to stderr via
> warnings.simplefilter('default', PendingDeprecationWarning)
I don't like this approach: it's a divergence from upstream, and it
seems like magic: it seems to be second-guessing the API call.
It seems simpler and clearer to ask that code that enables errors on
warnings need to turn it off for PendingDeprecationWarning to be able to
use modules that are known to break in the presence of the
PendingDeprecationWarning-treated-as-exceptions.
As long as you're saying turn off 'error' for all
PendingDeprecationWarnings, not just ones that are known to break,
I think we're saying the same thing in different words.
Here's how I see it working.
python-foo: Does not touch the warnings filters. No need to modify
anything.
pyhton-bar: Sets warnings.simplefilter('default'). No need to modify
anything
pyhon-baz: Sets warnings.simplefilter('errors'). Patch the code to set
warnings.simplefilter('default', PendingDeprecationWarning) and send a note
to upstream that 'errors' can cause segfaults in pieces of code unrelated to
python-baz.
python-qux: Contains code using the old PyCObject API and doesn't catch the
exception. No need to modify.
The key here, and only place where I'm not sure if we're saying the same
thing is that setting simplefilter('errors') can break unrelated code. So
python-baz might not use any code that breaks but python-corge might use
both python-baz and python-qux which breaks in this combination. So we
never want that to go through without adding a second filter somewhere. The
easiest place to track the adding of the second filter is in python-baz,
right after the filter that sets up 'errors'.
> > > > One issue here is that this API expresses a
binary interface between
> > > > different Python modules, and that we don't yet have a way to
express
> > > > this at the rpm metadata level. I think we should, to make it easier
to
> > > > track these issues in the future. I don't think it's
possible to track
> > > > these automatically, but we could do it manually.
> > > >
> > > Tracking this manualy is no good unless you can explain to people how to
> > > detect it. Once you can explain how to manually detect it, it might be
> > > possible to automatically detect it....
> >
> > You have to scrape through for ABI calls to PyCObject: the presence of
> > the calls are visible in the ELF metadata, but not the exact strings.
> > Actually, it _might_ be possible to figure them out via disassembly of
> > the machine code, but this seems fragile.
> >
> This already sounds like something that is too involved for maintainers and
> package reviewers to do. I think this might be something that doesn't leave
> the drawing board without tooling to at least do part of the detective work.
Fundamentally the review process for this involves grepping through the
source code; any usage of a PyCObject_* function will return NULL if
PendingDeprectationWarning is set to "error". It needs a little ability
to read C code, but (I hope) not much. The issue with macros in the
header files is a nasty gotcha, though :-(
What I'm commenting on here is the desire to stick all uses of the PyCapsule
API into an RPM virtual Provide and Require.
It doesn't seem like something that we can require of packagers and
reviewers without raising the barrier of entry for their skillset quite
a bit.
I had a go at writing some release notes for this issue here:
https://fedoraproject.org/wiki/Features/Python_2.7#Caveat:_PyCObject_and_...
How does this look?
Looks good but change:
- warnings.simplefilter('ignore', PendingDeprecationWarning)
+ warnings.simplefilter('default', PendingDeprecationWarning)
If the coder is going through the trouble of turning warnings into 'error',
they likely want to be at least warned about PendingDeprecationWarning as
well.
-Toshio
12:17 p.m.
On Mon, 2010-08-16 at 15:14 -0400, Toshio Kuratomi wrote:
On Mon, Aug 16, 2010 at 02:03:51PM -0400, David Malcolm wrote:
> On Fri, 2010-08-13 at 21:57 -0400, Toshio Kuratomi wrote:
> > > > On Fri, Aug 13, 2010 at 02:20:51PM -0400, David Malcolm wrote:
> > If this is okay, then I'd modify your point (a) to be this plan:
> >
> > When code that turns all warnings into errors is encountered, have it
> > instead cause PendingDeprecationWarning to print to stderr via
> > warnings.simplefilter('default', PendingDeprecationWarning)
>
> I don't like this approach: it's a divergence from upstream, and it
> seems like magic: it seems to be second-guessing the API call.
>
> It seems simpler and clearer to ask that code that enables errors on
> warnings need to turn it off for PendingDeprecationWarning to be able to
> use modules that are known to break in the presence of the
> PendingDeprecationWarning-treated-as-exceptions.
>
As long as you're saying turn off 'error' for all
PendingDeprecationWarnings, not just ones that are known to break,
I think we're saying the same thing in different words.
Here's how I see it working.
python-foo: Does not touch the warnings filters. No need to modify
anything.
pyhton-bar: Sets warnings.simplefilter('default'). No need to modify
anything
pyhon-baz: Sets warnings.simplefilter('errors'). Patch the code to set
warnings.simplefilter('default', PendingDeprecationWarning) and send a note
to upstream that 'errors' can cause segfaults in pieces of code unrelated to
python-baz.
python-qux: Contains code using the old PyCObject API and doesn't catch the
exception. No need to modify.
The key here, and only place where I'm not sure if we're saying the same
thing is that setting simplefilter('errors') can break unrelated code. So
python-baz might not use any code that breaks but python-corge might use
both python-baz and python-qux which breaks in this combination. So we
never want that to go through without adding a second filter somewhere. The
easiest place to track the adding of the second filter is in python-baz,
right after the filter that sets up 'errors'.
> > > > > One issue here is that this API expresses a binary interface
between
> > > > > different Python modules, and that we don't yet have a way
to express
> > > > > this at the rpm metadata level. I think we should, to make it
easier to
> > > > > track these issues in the future. I don't think it's
possible to track
> > > > > these automatically, but we could do it manually.
> > > > >
> > > > Tracking this manualy is no good unless you can explain to people how
to
> > > > detect it. Once you can explain how to manually detect it, it might
be
> > > > possible to automatically detect it....
> > >
> > > You have to scrape through for ABI calls to PyCObject: the presence of
> > > the calls are visible in the ELF metadata, but not the exact strings.
> > > Actually, it _might_ be possible to figure them out via disassembly of
> > > the machine code, but this seems fragile.
> > >
> > This already sounds like something that is too involved for maintainers and
> > package reviewers to do. I think this might be something that doesn't
leave
> > the drawing board without tooling to at least do part of the detective work.
>
> Fundamentally the review process for this involves grepping through the
> source code; any usage of a PyCObject_* function will return NULL if
> PendingDeprectationWarning is set to "error". It needs a little ability
> to read C code, but (I hope) not much. The issue with macros in the
> header files is a nasty gotcha, though :-(
>
What I'm commenting on here is the desire to stick all uses of the PyCapsule
API into an RPM virtual Provide and Require.
It doesn't seem like something that we can require of packagers and
reviewers without raising the barrier of entry for their skillset quite
a bit.
The presence is detectable though (via eu-readelf), so we could have an
rpmlint test for it (detect if a python extension calls PyCapsule_*, if
so, it must have a Requires/Provides on a capsule, though we can't tell
what it will be).
So we could implement the test, but point people needing help to fix it
at #fedora-python (or the python SIG list) where I can help them (and
others).
Having said all this, I'm not at all sure that this metadata would help
solve problems :)
> I had a go at writing some release notes for this issue here:
> https://fedoraproject.org/wiki/Features/Python_2.7#Caveat:_PyCObject_and_...
>
> How does this look?
>
Looks good but change:
- warnings.simplefilter('ignore', PendingDeprecationWarning)
+ warnings.simplefilter('default', PendingDeprecationWarning)
If the coder is going through the trouble of turning warnings into 'error',
they likely want to be at least warned about PendingDeprecationWarning as
well.
Thanks; I made the change
1:42 p.m.
On Fri, Aug 13, 2010 at 02:20:51PM -0400, David Malcolm wrote:
(Sorry about the length of this email)
Python 2.7 deprecated the PyCObject API in favor of a new "capsule" API.
http://docs.python.org/dev/whatsnew/2.7.html#capsules
(b) try to fix the ones that are self-contained; send fixes upstream
I couldn't really tell from the link above how one can migrate code.
Any suggestion how to change the code below? Variants of this are
used in both libvirt and libguestfs.
typedef struct {
PyObject_HEAD
guestfs_h *g;
} Pyguestfs_Object;
static guestfs_h *
get_handle (PyObject *obj)
{
assert (obj);
assert (obj != Py_None);
return ((Pyguestfs_Object *) obj)->g;
}
static PyObject *
put_handle (guestfs_h *g)
{
assert (g);
return
PyCObject_FromVoidPtrAndDesc ((void *) g, (char *) "guestfs_h", NULL);
}
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
1:19 p.m.
On Sat, 2010-08-14 at 19:42 +0100, Richard W.M. Jones wrote:
On Fri, Aug 13, 2010 at 02:20:51PM -0400, David Malcolm wrote:
> (Sorry about the length of this email)
>
> Python 2.7 deprecated the PyCObject API in favor of a new "capsule" API.
> http://docs.python.org/dev/whatsnew/2.7.html#capsules
> (b) try to fix the ones that are self-contained; send fixes upstream
I couldn't really tell from the link above how one can migrate code.
Any suggestion how to change the code below? Variants of this are
used in both libvirt and libguestfs.
typedef struct {
PyObject_HEAD
guestfs_h *g;
} Pyguestfs_Object;
static guestfs_h *
get_handle (PyObject *obj)
{
assert (obj);
assert (obj != Py_None);
return ((Pyguestfs_Object *) obj)->g;
}
FWIW, this code looks a little dubious to me: obj is actually a
PyCObject, which is:
typedef struct {
PyObject_HEAD
void *cobject;
void *desc;
void (*destructor)(void *);
} PyCObject;
It works because Pyguestfs_Object->g has the same offset from the top of
the struct as the PyCObject->cobject field, but if PyCObject had changed
layout it would crash.
Do you use Pyguestfs_Object elsewhere in the code? Looking at
libguestfs-1.5.2/src/generator.ml it doesn't seem to be, so it looks
like get_handle should have read:
static guestfs_h *
get_handle (PyObject *obj)
{
assert (obj);
assert (obj != Py_None);
return (guestfs_h*)PyCObject_AsVoidPtr(obj);
}
static PyObject *
put_handle (guestfs_h *g)
{
assert (g);
return
PyCObject_FromVoidPtrAndDesc ((void *) g, (char *) "guestfs_h", NULL);
}
Porting to the capsule API, I believe the code needs to look something
like this (caution: untested):
static guestfs_h *
get_handle (PyObject *obj)
{
assert (obj);
assert (obj != Py_None);
return (guestfs_h*)PyCapsule_GetPointer(obj, "guestfs_h");
}
static PyObject *
put_handle (guestfs_h *g)
{
assert (g);
return PyCapsule_New((void *) g, "guestfs_h", NULL);
}
1:25 p.m.
On Mon, 2010-08-16 at 14:19 -0400, David Malcolm wrote:
On Sat, 2010-08-14 at 19:42 +0100, Richard W.M. Jones wrote:
> On Fri, Aug 13, 2010 at 02:20:51PM -0400, David Malcolm wrote:
[snip]
Porting to the capsule API, I believe the code needs to look
something
like this (caution: untested):
static guestfs_h *
get_handle (PyObject *obj)
{
assert (obj);
assert (obj != Py_None);
return (guestfs_h*)PyCapsule_GetPointer(obj, "guestfs_h");
}
Note that if someone passes a capsule that doesn't have a "guestfs_h" in
its name slot, then PyCapsule_GetPointer will raise an exception i.e.
return NULL and set thread-local state: ideally your generated wrapped
code needs to handle that case.
[snip]
4:56 a.m.
Thanks for the tip. It's sort of annoying that this new API is not in
Python < 2.7, necessitating a bunch of #ifdef's in the code. This is
the patch I came up with:
https://www.redhat.com/archives/libguestfs/2010-August/msg00053.html
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
5000
days inactive
5004
days old
14 comments
5 participants
participants (5)
-
David Malcolm -
Jef Spaleta -
Kevin Kofler -
Richard W.M. Jones -
Toshio Kuratomi