-
Notifications
You must be signed in to change notification settings - Fork 431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix build of Python bindings with GCC 8 #518
Fix build of Python bindings with GCC 8 #518
Conversation
6e2a033
to
3e5ddff
Compare
That's why people SHOULD NOT use |
Sigh - CI errors are because this is an entirely new warning in GCC 8, so GCC 7 (which the CI uses) doesn't recognize |
@AdamWill just make it to not use -Werror when doing production builds, so you won't need |
It's not my project, so not my decision. |
3e5ddff
to
4d62d77
Compare
Stole an idea from openembedded to use GCC pragmas...this should work better I hope. |
GCC 8 appears to be rather stricter about various issues to do with type conversions and casts than GCC 7 was. This affects OpenColorIO's Python bindings quite heavily, producing a large number of warnings (which are converted to errors by `-Werror`) and outright errors. The changes here are almost all one of three basic types: 1. Many functions that become Python methods with no arguments (using the METH_NOARGS flag) did not include the expected second parameter in their signatures at all. METH_NOARGS does not prevent this second parameter being passed *at all*, it only ensures that it will always be NULL. It's still not technically correct to leave it out of the function signature; as a comment from 'yak' on https://stackoverflow.com/questions/10256315 points out, there are situations where this could cause a crash. I've added the second parameter (with no name, per convention) to every one of these cases. 2. In several cases, classes specified a custom destructor, with a cast to the `destructor` type, which only takes a single parameter. However, the signatures for these destructor functions included two parameters, assuming that they'd get an 'args' parameter (they do not). I've corrected all these cases. 3. In several cases, classes specified custom str or repr methods. However, in the `PyTypeObject` structures for these classes, these methods were not cast to the `reprfunc` type, as they ought to be. I've added these casts. There are two warnings I just can't get rid of with my limited C++ knowledge. The `Config` class (in PyConfig.cpp) defines a couple of methods that take kwargs as well as args. This is done by setting the `METH_KEYWORDS` flag, which ultimately seems to result in a cast from type `PyCFunctionWithKeywords` to `PyCFunction` happening somewhere behind the scenes. There's some discussion of this at https://stackoverflow.com/questions/9496753 GCC 8 does not like this cast - it causes a 'cast-function-type' warning. I've messed around a bit with `reinterpret_cast` and stuff, but didn't really understand precisely what I was doing and didn't manage to find anything that got rid of the warnings. So I just suppressed these warnings with GCC pragmas instead (using `-Wno-error=cast-function-type` causes older GCC versions to choke, so we can't do that). I've tested at least that the compilation succeeds, and I can import the Python module and instantiate a few classes and examine their docstrings and stuff with no apparent errors. Many thanks to Kevin Kofler for his help with these fixes. Signed-off-by: Adam Williamson <awilliam@redhat.com>
4d62d77
to
65f1ce0
Compare
Note: upstream https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01315.html / https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84531 should stop the remaining two warnings being emitted. Not sure what GCC version they'll land in. Thanks @davidmalcolm for the reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Job.
GCC 8 appears to be rather stricter about various issues to do with type conversions and casts than GCC 7 was. This affects OpenColorIO's Python bindings quite heavily, producing a large number of warnings (which are converted to errors by `-Werror`) and outright errors. The changes here are almost all one of three basic types: 1. Many functions that become Python methods with no arguments (using the METH_NOARGS flag) did not include the expected second parameter in their signatures at all. METH_NOARGS does not prevent this second parameter being passed *at all*, it only ensures that it will always be NULL. It's still not technically correct to leave it out of the function signature; as a comment from 'yak' on https://stackoverflow.com/questions/10256315 points out, there are situations where this could cause a crash. I've added the second parameter (with no name, per convention) to every one of these cases. 2. In several cases, classes specified a custom destructor, with a cast to the `destructor` type, which only takes a single parameter. However, the signatures for these destructor functions included two parameters, assuming that they'd get an 'args' parameter (they do not). I've corrected all these cases. 3. In several cases, classes specified custom str or repr methods. However, in the `PyTypeObject` structures for these classes, these methods were not cast to the `reprfunc` type, as they ought to be. I've added these casts. There are two warnings I just can't get rid of with my limited C++ knowledge. The `Config` class (in PyConfig.cpp) defines a couple of methods that take kwargs as well as args. This is done by setting the `METH_KEYWORDS` flag, which ultimately seems to result in a cast from type `PyCFunctionWithKeywords` to `PyCFunction` happening somewhere behind the scenes. There's some discussion of this at https://stackoverflow.com/questions/9496753 GCC 8 does not like this cast - it causes a 'cast-function-type' warning. I've messed around a bit with `reinterpret_cast` and stuff, but didn't really understand precisely what I was doing and didn't manage to find anything that got rid of the warnings. So I just suppressed these warnings with GCC pragmas instead (using `-Wno-error=cast-function-type` causes older GCC versions to choke, so we can't do that). I've tested at least that the compilation succeeds, and I can import the Python module and instantiate a few classes and examine their docstrings and stuff with no apparent errors. Many thanks to Kevin Kofler for his help with these fixes. Signed-off-by: Adam Williamson <awilliam@redhat.com>
GCC 8 appears to be rather stricter about various issues to do
with type conversions and casts than GCC 7 was. This affects
OpenColorIO's Python bindings quite heavily, producing a large
number of warnings (which are converted to errors by
-Werror
)and outright errors.
The changes here are almost all one of three basic types:
Many functions that become Python methods with no arguments
(using the METH_NOARGS flag) did not include the expected second
parameter in their signatures at all. METH_NOARGS does not
prevent this second parameter being passed at all, it only
ensures that it will always be NULL. It's still not technically
correct to leave it out of the function signature; as a comment
from 'yak' on https://stackoverflow.com/questions/10256315
points out, there are situations where this could cause a crash.
I've added the second parameter (with no name, per convention)
to every one of these cases.
In several cases, classes specified a custom destructor, with
a cast to the
destructor
type, which only takes a singleparameter. However, the signatures for these destructor functions
included two parameters, assuming that they'd get an 'args'
parameter (they do not). I've corrected all these cases.
In several cases, classes specified custom str or repr
methods. However, in the
PyTypeObject
structures for theseclasses, these methods were not cast to the
reprfunc
type, asthey ought to be. I've added these casts.
There are two warnings I just can't get rid of with my limited
C++ knowledge. The
Config
class (in PyConfig.cpp) defines acouple of methods that take kwargs as well as args. This is
done by setting the
METH_KEYWORDS
flag, which ultimately seemsto result in a cast from type
PyCFunctionWithKeywords
toPyCFunction
happening somewhere behind the scenes. There's somediscussion of this at https://stackoverflow.com/questions/9496753
GCC 8 does not like this cast - it causes a 'cast-function-type'
warning. I've messed around a bit with
reinterpret_cast
andstuff, but didn't really understand precisely what I was doing
and didn't manage to find anything that got rid of the warnings.
So I just added
-Wno-error=cast-function-type
to the compilerflags instead, which turns these back into warnings and allows
the compilation to succeed.
I've tested at least that the compilation succeeds with only
those two warnings, and I can import the Python module and
instantiate a few classes and examine their docstrings and stuff
with no apparent errors or crashes.
Signed-off-by: Adam Williamson awilliam@redhat.com