This patch compiles and works under Python 3 and 2.6. I haven't tested with < 2.6 but I figure we can deprecate support for < 2.6 and tell people to use the old bindings since nothing really new has been added. We can revisit if there is a major overhaul. We should release this as a beta because there might be issues with the change to default Unicode encoding. Otherwise it should be pretty much identical to the older version. The original fedora bugs can be seen here: https://bugzilla.redhat.com/show_bug.cgi?id=538616 Moving the patch development upstream to Freedesktop.org. Fedora bug should now be used only to track packaging issue and fedora/RHEL related issues.
Created attachment 33044 [details] [review] Patch to make dbus-python compile and work in python 3
Simon, if you give me the ok, I'll create a git branch and add the patch so it can be vetted and then eventually merged into mainline.
I haven't reviewed the patch, but you're welcome to create a git branch for it; review is only needed for merges to a "releasable" branch (of which dbus-python currently only has master), IMO. I'd rather continue to support Python 2.5 if possible, since that's the version in the stable releases of Debian, Ubuntu and Maemo.
Cool, I just wanted to get one other person to sign off on it before I touched a repo that I haven't touched in years. Note that I didn't say it wouldn't work in 2.5, only that I haven't tested it and until I get my new machine, I don't have disk space to install another OS so if someone can test it for now that would be great.
Hmmm, I can't create the branch. 'Read-only file system' even though I cloned through ssh. Am I still in the commit group?
Yep: walters@annarchy:~$ groups johnp freedesktop dbus walters@annarchy:~$ Are you sure you have the right checkout url?
git clone git+ssh://johnp@git.freedesktop.org/git/dbus/dbus-python.git
Works now. I left off the git. in git.freedesktop.org. Changed my url to point to git.freedesktop.org instead of freedesktop.org and it works now
Patch pushed to branch py3k along with a fix for the _dbus_glib_bindings module For those who want to help out switch to the py3k branch by cloning the dbus-python repo (or use your already cloned repo) and track the branch as such: git checkout -tb py3k origin/py3k
(In reply to comment #9) > Patch pushed to branch py3k along with a fix for the _dbus_glib_bindings module > > For those who want to help out switch to the py3k branch by cloning the > dbus-python repo (or use your already cloned repo) and track the branch as > such: > > git checkout -tb py3k origin/py3k > Thanks for your work on this. I hope that it will possible to merge this, so that the python2 and python3 bindings can be built from the same sources. Python 2.6 contains some compatibility APIs that make this easier, but I've found it's normally trivial to support the older minor releases of Python 2 using some preprocessor defines.
Did a bunch of work and fixed all the issues for Python 2.6 (e.g. it passes make check). Simon, one thing that stumped me was the different ways we extend base types for variant_level. Why are some done with C extensions to the struct and others use attr? I noticed with the long object trying to covert it to a struct extension made the values you put in off by a power of two when you go to print it out. I'm having issue compiling under 3.1. I get this: libtool: link: gcc -shared .libs/abstract.o .libs/bus.o .libs/bytes.o .libs/conn.o .libs/conn-methods.o .libs/containers.o .libs/debug.o .libs/exceptions.o .libs/float.o .libs/generic.o .libs/int.o .libs/libdbusconn.o .libs/mainloop.o .libs/message-append.o .libs/message.o .libs/message-get-args.o .libs/module.o .libs/pending-call.o .libs/server.o .libs/signature.o .libs/string.o .libs/validation.o -L/lib -ldbus-1 -Wl,-soname -Wl,_dbus_bindings.so -Wl,-version-script -Wl,.libs/_dbus_bindings.ver -o .libs/_dbus_bindings.so /usr/bin/ld:.libs/_dbus_bindings.ver:2: syntax error in VERSION script I think it has something to do with moving from init_dbus_bindings to PyInit__dbus_bindings. It seems to not find any public methods. For instance in the 2.6 version the .ver file has this in it: { global: init_dbus_bindings; local: *; }; In 3.1 it has this: { global: local: *; }; Not sure how to fix that. We need to also think how we are going to support dual targets in the same autotools files and in the python files. David suggested running 2to3 during compile time.
fixed the .ver file issue with this in Makefile.am -export-symbols-regex (PyInit_|init)_dbus_bindings
Alright, it is compiling under both versions and passing the tests on 2.6. Right now we have some issues with the bytes/unicode changes that need discussion. Since dbus specifies that signatures, object_paths, service names, etc. are all ascii I have decided to make them all subclass the Bytes type. This make it much faster to extract them as we don't have to encode to bytes first and then extract the char *, but b'a' != 'a' in the test cases. We could have separate test cases for 2.x and 3.x or we could just move to Unicode for everything. I'm not sure what is best.
> (In reply to comment #9) > I hope that it will possible to merge this, so that the python2 and python3 > bindings can be built from the same sources. I hope so too, but only when it's ready. Please don't consider the py3k branch to be an API-stable platform until it's merged into master and included in a numbered release (which, IMO, it shouldn't be yet). I'm fairly sure it will need some work on the type system, which I'm afraid I don't have time to do right now; I should be able to devote more time to this in a couple of weeks. (In reply to comment #11) > Did a bunch of work and fixed all the issues for Python 2.6 (e.g. it passes > make check). Simon, one thing that stumped me was the different ways we extend > base types for variant_level. Why are some done with C extensions to the > struct and others use attr? I noticed with the long object trying to covert it > to a struct extension made the values you put in off by a power of two when you > go to print it out. That's because long is a "variable-length object" that can't safely extend the C struct, whereas int is "fixed-length object" that can be extended in the same way you're used to for GObject. Please read and understand the CPython API docs before hacking on the C extension! :-) Briefly: the 'str' C struct ends with a char[1] or some such, which is actually a placeholder representing a char[n] for a suitably large n. If you put the variant_level member after the char[1], but assign a bigger-than-1-character value to it, the extra character data will overflow into, and corrupt, the variant_level. 'long' and 'unicode' behave like 'str', except that their "characters" are integers representing segments of the arbitrary-length integer it represents. However, Python 2's 'int' is fixed-length, and 'list' and presumably Python 3's 'bytes' store their data in a separately-allocated array like you'd expect in a GObject (they have to, because they're mutable). For such types, it's safe (and somewhat more efficient) to extend the struct, rather than attaching a __dict__. Extending the struct corresponds to using __slots__ all the way down a hierarchy of pure-Python subtypes, whereas attaching a __dict__ corresponds to not using __slots__. (In reply to comment #13) > Since dbus specifies that signatures, object_paths, service names, etc. are all > ascii I have decided to make them all subclass the Bytes type. I don't think that's right; they should all subclass str, the normal Python string type (which happens to be Unicode on Python 3). However, dbus.UTF8String should be a deprecated subtype of bytes when running on Python 3, and dbus.ByteArray should be a subtype of bytes. It's probably also worth setting byte_arrays=True by default in Python 3 (the ability to use a dbus.Array of dbus.Byte is almost never useful), and it might even be worth removing utf8_strings and UTF8String altogether.
(In reply to comment #14) > (In reply to comment #13) > > Since dbus specifies that signatures, object_paths, service names, etc. are all > > ascii I have decided to make them all subclass the Bytes type. > > I don't think that's right; they should all subclass str, the normal Python > string type (which happens to be Unicode on Python 3). Easy enough to do but it entails doing something like this every time we want to access the raw char pointer: PyObject *sig_utf8 = PyUnicode_AsUTF8String(sig); char *signature = PyBytes_AsString(sig_utf8); /*do something with signature */ Py_DECREF(signature); > However, dbus.UTF8String should be a deprecated subtype of bytes when running > on Python 3, and dbus.ByteArray should be a subtype of bytes. Ok. > It's probably also worth setting byte_arrays=True by default in Python 3 (the > ability to use a dbus.Array of dbus.Byte is almost never useful), and it might > even be worth removing utf8_strings and UTF8String altogether. I think this is a good idea to simplify things. What do you think about the change to subtyping from Long on both 2.x and 3.x which gets rid of any Int calls. It passes the test suite.
(In reply to comment #15) > What do you think about the change to subtyping from Long on both 2.x and 3.x > which gets rid of any Int calls. It passes the test suite. I don't think that's a good idea on 2.x; changing the inheritance hierarchy is an API break.
(In reply to comment #16) > (In reply to comment #15) > > What do you think about the change to subtyping from Long on both 2.x and 3.x > > which gets rid of any Int calls. It passes the test suite. > > I don't think that's a good idea on 2.x; changing the inheritance hierarchy is > an API break. In which case why am I bothering trying to support both version in one codebase if we can't unify them as much as possible? The old codebase hasn't been touched in some time. Why are we supporting internals such as abstract.c as part of the API guarantees. It just seems like a huge maintenance nightmare and both of us have shone that we have little time to maintain this codebase beyond basic bug fixing. I've always maintained that the only API guarantees we give are on the higher level Python bits.
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > What do you think about the change to subtyping from Long on both 2.x and 3.x > > > which gets rid of any Int calls. It passes the test suite. > > > > I don't think that's a good idea on 2.x; changing the inheritance hierarchy is > > an API break. > > In which case why am I bothering trying to support both version in one codebase > if we can't unify them as much as possible? The old codebase hasn't been > touched in some time. Why are we supporting internals such as abstract.c as > part of the API guarantees. It just seems like a huge maintenance nightmare > and both of us have shone that we have little time to maintain this codebase > beyond basic bug fixing. I've always maintained that the only API guarantees > we give are on the higher level Python bits. After thinking about it a bit I can revert the Int class but it needs to be changed to use __dict__ for the signature so the code in other areas doesn't have to be branched. I understand the need for someone to be able to check the base type. In Python 3.x I will just alias the DBusInt base class to the DBusLong base class.
Looks like I am getting close. I reverted the 2.x version to inheriting from the correct string and number class (e.g. PyString and PyInt) and it passes the test suite. The 3.x has one more test it isn't passing in the standalone tests which has to do with guess_signature not handling the String (2.x) vs. Unicode (3.x) correctly. It is a bit of a messy change so I'm thinking about the best way to support both versions (e.g. two separate functions, a bunch of #if conditionals or macros).
test-standalone.py now passes under 3.x however since the rest of the tests require pygobject we are kind of screwed until that is ported. We have a hackfest in Boston in April coming up to do just that. I might also provide alternate tests based on the busy loop dispatching code in libdbus if I have time. Also, apparently the 'sets' module doesn't seem to be available in 3.x. As for 2.x everything seems to be running there fine. It still is passing tests and we even use the standard PyInt and PyString base classes so it should be API equivalent to the current bindings when compiled under 2.x (internally the PyDBusIntBase class now stores variant_level in its dict, the same way PyDBusLongBase does). One question does remain. How do we handle the pure python files in a clean way? Do we run them through 2to3 during compile? Do we ship two different directories for 2.x and 3.x files? Do we move them to a src directory and on 2.x just move the file and on 3.x, run 2to3 and have them output to the module directory?
Hi, I'm posting here because John's blog seems closed for comments. I needed the following patch on top of John's py3k branch to make 0.83.1 bindings work: http://www.openstreetmap.pl/balrog/dbus-python-0.83.1-py3k.patch In short I took 0.83.1 and applied the diff between py3k and master on top of it and then the patch above. I'm a little clueless about python and dbus, but you'll notice the patch changes some uses of PyBytes_FromString to PyUnicode_FromString -- this was required to make signals work, otherwise a .decode("utf-8") was needed on the python side because the strings now became bytes. I'm not sure why the change from PyString to PyBytes was made. Instead I think python-2to3-macros.h should be #defining PyString_FromString to PyUnicode_FromString for python 3, and leaving it as is for python 2. I can send a patch to do something like that if needed.
(In reply to comment #21) This is the correct place for patches. I use Bytes there because the spec only allows characters in the ASCII range (it is mapped to String for Python 2.x). I have no issue with having it be Unicode for 3.x though. As for the Python source files I haven't decided how we want to handle 2.x compatibility. It was my thought that the build would run 2to3 on intermediate files. The source needs to run on at least Python 2.5 if not 2.4. We want to merge into master as soon as possible. In any case I'm working with pygi right now but once I am done I plan on running the test cases using the new py3k branch of pygobject. I will also decide how we are going to support 2.x and 3.x from the same source at this time.
(In reply to comment #18) > After thinking about it a bit I can revert the Int class but it needs to be > changed to use __dict__ for the signature so the code in other areas doesn't > have to be branched. I understand the need for someone to be able to check the > base type. In Python 3.x I will just alias the DBusInt base class to the > DBusLong base class. That sounds good. (In reply to comment #21) > you'll notice the patch changes some uses of PyBytes_FromString to > PyUnicode_FromString Yes, I think object paths and signatures should be String in Python 3, because they're conceptually strings, not blobs of bytes (the same reason that identifiers in source code are still str, even though str now means what unicode used to mean).
hi guys i want to build dbus-python3 but i had some error when i did the flowing plz tell me if i have something wrong 1-git clone git://anongit.freedesktop.org/dbus/dbus-python 2-git checkout -tb py3k origin/py3k 3-git pull 4-PYTHON=/usr/bin/python3 ./autogen.sh 5-make 6-sudo make install but it gave me lot's of error like " Byte-compiling python modules... dbus/bus.py File "/usr/local/lib/python3/dist-packages/dbus/bus.py", line 179 except DBusException, e: ^ SyntaxError: invalid syntax " thanks in advanced :)
My guess is the patch just bitrot. I haven't had time to touch it for a bit. Now that we have PyGI running in Py3k we could run the D-Bus test suite in full. Not sure if I will have much time for this though. As for your error it can be fixed with this code: import sys try: ... except DBusException: info, e = sys.exc_info[:2] ...
thanks for your reply... where i should put the code you provide??? this error " Byte-compiling python modules... dbus/bus.py File "/usr/local/lib/python3/dist-packages/dbus/bus.py", line 179 except DBusException, e: ^ SyntaxError: invalid syntax " it appear when i do step 6 of installation so the install process of dbus-python didn't complete so it's something connected to the patched version of dbus-python or i did something wrong in the build/install steps i need dbus-python3 so i can build pyqt for python3 with dbus enabled thanks again :)
Created attachment 39800 [details] [review] Fix Raise and except to match python 3.x semantics
Comment on attachment 39800 [details] [review] Fix Raise and except to match python 3.x semantics fixes problems recently complained about with compiling this
(In reply to comment #26) > thanks for your reply... > > where i should put the code you provide??? > See my patch I just made to fix the problem. I figured the best was was to changed except blah, something: to except blah as something: as that is what py3 wants. Also I fixed one raise which was using old string exception semantics. I kept them as a single commit as it is all fixes to the exception handling.
Actually using "as" doesn't work because it breaks older versions. Ideally we should be running from the same source on Python 3 and Python 2. I don't think we can assume everyone has moved to 2.6/2.7 which brings some of the Python 3 constructs to Python 2
(In reply to comment #30) > Actually using "as" doesn't work because it breaks older versions. Ideally we > should be running from the same source on Python 3 and Python 2. I don't think > we can assume everyone has moved to 2.6/2.7 which brings some of the Python 3 > constructs to Python 2 You are correct my change would not work pre 2.6. The problems are deeper than that though. I just fired it up under python 3.2a3 and I get : >>> import dbus Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/opt/Python-3.2a3/lib/python3.2/site-packages/dbus/__init__.py", line 79, in <module> import dbus.types as types File "/opt/Python-3.2a3/lib/python3.2/site-packages/dbus/types.py", line 6, in <module> from _dbus_bindings import ObjectPath, ByteArray, Signature, Byte,\ ImportError: /opt/Python-3.2a3/lib/python3.2/site-packages/_dbus_bindings.so: undefined symbol: PyCObject_FromVoidPtr After some research I came to learn PyCObject is deprecated in 2.7 and removed in 3.2 in favour of PyCapsule (as its C code it can probably be fixed with #if's, but its getting messy). I am just trying to learn what the changes actually entail now.
(In reply to comment #28) > (From update of attachment 39800 [details] [review]) > fixes problems recently complained about with compiling this thanks for you patch file i finally can build dbus-python3 but i had error when trying to import it >>> import dbus no module named dummy_thread it was easy for me to figure it out that python3 changed the module thread to _thread so all i had to do is replacing dummy_thread with _thread and now i can import it
(In reply to comment #31) > (In reply to comment #30) > After some research I came to learn PyCObject is deprecated in 2.7 and removed > in 3.2 in favour of PyCapsule (as its C code it can probably be fixed with > #if's, but its getting messy). I am just trying to learn what the changes > actually entail now. I tried the following in module.c: #if (PY_MAJOR_VERSION > 3) || ((PY_MAJOR_VERSION == 3) && (PY_MINOR_VERSION >= 2)) c_api = PyCapsule_New((void *)dbus_bindings_API, "dbus._C_API", NULL); #else c_api = PyCObject_FromVoidPtr ((void *)dbus_bindings_API, NULL); #endif it gets me further: >>> import dbus Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/opt/Python-3.2a3/lib/python3.2/site-packages/dbus/__init__.py", line 78, in <module> import dbus.exceptions as exceptions AttributeError: 'module' object has no attribute 'exceptions' After several hours I haven't been able to work this one out, so at this stage getting d-bus python to work on 3.2a3 has defeated me :( Im happy to recut my patch using the construct given above instead of as, but without solving this issue it seems a moot point. Unless the line is drawn as 3.1? Personally as python "language" is stable between 3.1 and 3.2 I think there will be an expectation that stuff that works on 3.1 should also work on 3.2, even though the C api's have changed.
(In reply to comment #33) > (In reply to comment #31) > > (In reply to comment #30) > > > After some research I came to learn PyCObject is deprecated in 2.7 and removed > > in 3.2 in favour of PyCapsule (as its C code it can probably be fixed with > > #if's, but its getting messy). I am just trying to learn what the changes > > actually entail now. > > I tried the following in module.c: > > #if (PY_MAJOR_VERSION > 3) || ((PY_MAJOR_VERSION == 3) && (PY_MINOR_VERSION >= > 2)) > c_api = PyCapsule_New((void *)dbus_bindings_API, "dbus._C_API", NULL); > #else > c_api = PyCObject_FromVoidPtr ((void *)dbus_bindings_API, NULL); > #endif This is sort of correct. See my patch for PyCairo - https://bugs.freedesktop.org/show_bug.cgi?id=30289 BTW you can apply PyCapsule to 3.0 and above since 3.0 will never be supported, and 3.1 introduced the API. You can also apply it to 2.7 and above if it doesn't appear in any header files (which would require other modules to recompile). I don't think any modules compile against the D-Bus python module so we are golden. This will ensure people don't get aborts if their warning level is set too high. Note that the name you give has to conform to a certain standard which corresponds to where you add the attr to the object for any PyCapsule_Import call to work properly. I think your patch looks correct. > it gets me further: > > >>> import dbus > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > File "/opt/Python-3.2a3/lib/python3.2/site-packages/dbus/__init__.py", line > 78, in <module> > import dbus.exceptions as exceptions > AttributeError: 'module' object has no attribute 'exceptions' > > After several hours I haven't been able to work this one out, so at this stage > getting d-bus python to work on 3.2a3 has defeated me :( Im happy to recut my > patch using the construct given above instead of as, but without solving this > issue it seems a moot point. Unless the line is drawn as 3.1? Personally as > python "language" is stable between 3.1 and 3.2 I think there will be an > expectation that stuff that works on 3.1 should also work on 3.2, even though > the C api's have changed. It needs to work on 3.2 since that is what we will ship in Fedora 15 in approx 6-8 months. Thanks for the extra work on this. Is there an exceptions.py in /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/? Sounds like a path issue to me. Do you have commit access? Can you commit your patches (with the "as" issue fixed) to the py3k branch?
Comment on attachment 33044 [details] [review] Patch to make dbus-python compile and work in python 3 look at the py3k branch in git. this patch is outdated
(In reply to comment #34) > BTW you can apply PyCapsule to 3.0 and above since 3.0 will never be supported, > and 3.1 introduced the API. You can also apply it to 2.7 and above if it > doesn't appear in any header files The whole point of the PyCObject is to access dbus-python's small C API via a header file. PyQt (third-party) uses this to provide Qt main loop integration. (_dbus_glib_bindings in dbus-python also uses it for GLib main loop integration, but that's in-tree.)
(In reply to comment #36) > (In reply to comment #34) > > BTW you can apply PyCapsule to 3.0 and above since 3.0 will never be supported, > > and 3.1 introduced the API. You can also apply it to 2.7 and above if it > > doesn't appear in any header files > > The whole point of the PyCObject is to access dbus-python's small C API via a > header file. PyQt (third-party) uses this to provide Qt main loop integration. > > (_dbus_glib_bindings in dbus-python also uses it for GLib main loop > integration, but that's in-tree.) Ah, in which case we should only apply this to 3.x since we haven't made a release of it yet. That way we don't break ABI. Simon, if I put some more work into this can you find some time to review it and finally get it all merged and released?
(In reply to comment #34) > (In reply to comment #33) > > (In reply to comment #31) > > > (In reply to comment #30) > > > > > After some research I came to learn PyCObject is deprecated in 2.7 and removed > > > in 3.2 in favour of PyCapsule (as its C code it can probably be fixed with > > > #if's, but its getting messy). I am just trying to learn what the changes > > > actually entail now. > > > > I tried the following in module.c: > > > > #if (PY_MAJOR_VERSION > 3) || ((PY_MAJOR_VERSION == 3) && (PY_MINOR_VERSION >= > > 2)) > > c_api = PyCapsule_New((void *)dbus_bindings_API, "dbus._C_API", NULL); > > #else > > c_api = PyCObject_FromVoidPtr ((void *)dbus_bindings_API, NULL); > > #endif > > This is sort of correct. See my patch for PyCairo - > https://bugs.freedesktop.org/show_bug.cgi?id=30289 > > BTW you can apply PyCapsule to 3.0 and above since 3.0 will never be supported, > and 3.1 introduced the API. Wouldn't it be better therefore to just make this change for >= 3.1. Even though 3.0 isn't supported is it really necessary to break it knowingly? > > > it gets me further: > > > > >>> import dbus > > Traceback (most recent call last): > > File "<stdin>", line 1, in <module> > > File "/opt/Python-3.2a3/lib/python3.2/site-packages/dbus/__init__.py", line > > 78, in <module> > > import dbus.exceptions as exceptions > > AttributeError: 'module' object has no attribute 'exceptions' > > [...] > > It needs to work on 3.2 since that is what we will ship in Fedora 15 in approx > 6-8 months. Nice. > Thanks for the extra work on this. Is there an exceptions.py in > /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/? Sounds like a path issue > to me. ls /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/exceptions.p* /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/exceptions.py /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/exceptions.pyo /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/exceptions.pyc The error was making me think I needed to tell the module about exceptions in the c api somehow, but as I said I couldn't find anything on it or even a good reference for the error. > Do you have commit access? Can you commit your patches (with the "as" > issue fixed) to the py3k branch? No, i don't have commit access as far as I know. I can post a revised patch later today however.
(In reply to comment #38) > (In reply to comment #34) > > (In reply to comment #33) > > > (In reply to comment #31) > > > > (In reply to comment #30) > > > > > > > After some research I came to learn PyCObject is deprecated in 2.7 and removed > > > > in 3.2 in favour of PyCapsule (as its C code it can probably be fixed with > > > > #if's, but its getting messy). I am just trying to learn what the changes > > > > actually entail now. > > > > > > I tried the following in module.c: > > > > > > #if (PY_MAJOR_VERSION > 3) || ((PY_MAJOR_VERSION == 3) && (PY_MINOR_VERSION >= > > > 2)) > > > c_api = PyCapsule_New((void *)dbus_bindings_API, "dbus._C_API", NULL); > > > #else > > > c_api = PyCObject_FromVoidPtr ((void *)dbus_bindings_API, NULL); > > > #endif > > > > This is sort of correct. See my patch for PyCairo - > > https://bugs.freedesktop.org/show_bug.cgi?id=30289 > > > > BTW you can apply PyCapsule to 3.0 and above since 3.0 will never be supported, > > and 3.1 introduced the API. > > Wouldn't it be better therefore to just make this change for >= 3.1. Even > though 3.0 isn't supported is it really necessary to break it knowingly? 3.0 is broken anyway. Also we should be specifying a minimum version of 3.1 in configure.ac, so it is much cleaner if the macro looks the same as all the others. It makes it much easier to search on to validate the 3.x specific code. I might end up changing all of that to use the HEX values as we do in pygobject. > > > > > it gets me further: > > > > > > >>> import dbus > > > Traceback (most recent call last): > > > File "<stdin>", line 1, in <module> > > > File "/opt/Python-3.2a3/lib/python3.2/site-packages/dbus/__init__.py", line > > > 78, in <module> > > > import dbus.exceptions as exceptions > > > AttributeError: 'module' object has no attribute 'exceptions' > > > > [...] > > > > It needs to work on 3.2 since that is what we will ship in Fedora 15 in approx > > 6-8 months. > Nice. > > > Thanks for the extra work on this. Is there an exceptions.py in > > /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/? Sounds like a path issue > > to me. > > ls /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/exceptions.p* > > /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/exceptions.py > /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/exceptions.pyo > /opt/Python-3.2a3/lib/python3.2/site-packages/dbus/exceptions.pyc > > The error was making me think I needed to tell the module about exceptions in > the c api somehow, but as I said I couldn't find anything on it or even a good > reference for the error. > > > Do you have commit access? Can you commit your patches (with the "as" > > issue fixed) to the py3k branch? > > No, i don't have commit access as far as I know. I can post a revised patch > later today however. Ah, I think I know what the issue is. I run into it a lot. Basically if there is an error in the exceptions.py module it can't import it so it gives you the can't import error. This usually happens when the exception happens deep with the code. If you look at the traceback it might have the real reason for not being able to import the module. Most likely a 3.2 syntax error.
Hi, guys. What is the state of this porting? I see that the last patch on py3k branch was from feb/2010. Is there any thing I could help? I don't have much experience with dbus or C, but I'm willing to learn (I know enough to be useful and will learn it quickly and without much tutoring on your part).
Someone needs to bring the py3k branch up to date with head, apply this patch, get the tests working and get it reviewed and merged with master. Most of this has be supplanted by the gdbus work done in pygobject which provides python 3 bindings to dbus through GLib's Gio library. Admittedly it lacks the features of D-Bus Python but is a lot easier to maintain.
I'd recommend using GDBus for D-Bus in Python 3 code. dbus-python does not follow the principle of "In the face of ambiguity, refuse the temptation to guess", and can't be changed to not do so without seriously breaking compatibility. Moving from Python 2 to Python 3 seems as good a time as any to switch... Redesigning dbus-python to make the type system explicit would basically end up with something resembling GDBus (but implemented on top of a less tractable library with worse thread-safety), and nobody's actively working on it anyway.
(In reply to comment #42) > I'd recommend using GDBus for D-Bus in Python 3 code. dbus-python does not > follow the principle of "In the face of ambiguity, refuse the temptation to > guess", and can't be changed to not do so without seriously breaking > compatibility. > > Moving from Python 2 to Python 3 seems as good a time as any to switch... > > Redesigning dbus-python to make the type system explicit would basically end up > with something resembling GDBus (but implemented on top of a less tractable > library with worse thread-safety), and nobody's actively working on it anyway. I see... But it would make dbus dependent on glib. For example, PyQt doesn't expose Qt's support for dbus, for it being too much "C== oriented", according to this: http://www.riverbankcomputing.co.uk/static/Docs/PyQt4/html/introduction.html#pyqt-components. Is it possible to use GDbus without GLib main loop?
(In reply to comment #43) > (In reply to comment #42) > > I'd recommend using GDBus for D-Bus in Python 3 code. dbus-python does not > > follow the principle of "In the face of ambiguity, refuse the temptation to > > guess", and can't be changed to not do so without seriously breaking > > compatibility. > > > > Moving from Python 2 to Python 3 seems as good a time as any to switch... > > > > Redesigning dbus-python to make the type system explicit would basically end up > > with something resembling GDBus (but implemented on top of a less tractable > > library with worse thread-safety), and nobody's actively working on it anyway. > > I see... But it would make dbus dependent on glib. For example, PyQt doesn't > expose Qt's support for dbus, for it being too much "C== oriented", according > to this: > http://www.riverbankcomputing.co.uk/static/Docs/PyQt4/html/introduction.html#pyqt-components. > Is it possible to use GDbus without GLib main loop? No, it runs using gio which is tied to the mainloop for async io. There is integration between Qt and the glib mainloop but I'm not sure it works or not. My guess is Qt needs to have it's own integrated python-dbus libraries or someone start maintaining these. The advantage of rewriting is you can have tighter integration with Qt and you can break some of the more complicated stuff making it easier to maintain. The other option is to fork dbus-python or finish the branch. I went a long way to getting this to run in python 3 but I no longer have time or have even really touched d-bus in the last year.
Making dbus depend on Gnome doesn't seem like a good approach for KDE users.
(In reply to comment #44) > > Is it possible to use GDbus without GLib main loop? > > No, it runs using gio which is tied to the mainloop for async io. There is > integration between Qt and the glib mainloop but I'm not sure it works or not. FWIW, Qt/GLib main loop integration does work: Qt4 in Debian (and derivatives, like Ubuntu) is compiled with it enabled, and we use it for telepathy-qt4's regression tests, in which QtDBus clients talk to dbus-glib services in the same process. (In reply to comment #45) > Making dbus depend on Gnome doesn't seem like a good approach for KDE users. Making dbus-python (a subset of D-Bus) depend on GLib (a library used by GNOME) isn't the same thing as making D-Bus depend on GNOME. As a result of the main-loop integration, Qt (and hence KDE) on Debian/Ubuntu already depends on GLib.
> (In reply to comment #45) > > Making dbus depend on Gnome doesn't seem like a good approach for KDE users. > > Making dbus-python (a subset of D-Bus) depend on GLib (a library used by GNOME) > isn't the same thing as making D-Bus depend on GNOME. As a result of the > main-loop integration, Qt (and hence KDE) on Debian/Ubuntu already depends on > GLib. To be clear: D-Bus doesn't depend on Glib either. It's only dbus-python that we're discussing here. Therefore, Qt doesn't depend on Glib -- it's an optional dependency that may be turned off at compile-time and at run-time.
To clear things up further, we are not forcing people to use GLib integration. It is up to KDE and Qt people to decide how they want to support D-Bus in python. There are plenty of options here, the GLib integration being one of them. What happened was I no longer work on D-Bus. Any work I do in that area is purely because I see a need and take time out to lend my knowledge to the situation. I wrote the Py3k branch because we were making a push for Py3k support in Fedora and upstream GNOME. When I realized we needed the glib mainloop in pygobject to be ported to py3k to run the complete test suite I switched focus to that. We decided in that project to switch from statically binding APIs to using the new GObject Introspection libraries. That almost instantly opened up the GDBus API to python. A small shim was written by Martin Pitti to bring some of the nicer syntax from DBus-Python. It means less maintenance for us in the long run as well as better resource usage. In any case while it would be nice to support a generic library for D-Bus in Python 3, the parts that I had time for are already solved on my end. I'm sure Simon, with everything that is on his plate, is in the same position. I've already done most of the heavy lifting to porting this library to Py3k. So there are options for Qt support when moving to Python 3. Someone just has to step up to the plate and spend the time on it to the level that the past maintainers have already put in (e.g. you can't just code dump and walk away, someone needs to support this). DBus-Python will still work and be supported for some time to come under Python 2. Let's not start bike-shedding the issue.
All right. I believe the best choice is to try to adapt QtDbus to Python. I'll contact PySide people and volunteer on that. First they need to support Python 3 as well, but that is another matter. Thanks everyone for the kind and quick answers.
*** Bug 37982 has been marked as a duplicate of this bug. ***
I'm following up to this thread because I'd like to resurrect dbus-python support for Python 3. I've got an experimental branch in bzr. lp:~barry/python-dbus/py3. I've read this bug thread and taken a look at the various patches posted here, and my branch does make some different choices. - Python 2.6, 2.7, and 3.2 are supported, nothing older. - I did original make the various ascii-string types as bytes subclasses, and while I was able to get test-standalone working, the cross-tests have had various difficult to track down failures. I'm going to do another branch where they are str/unicode subclasses and see if that makes things better. Conceptually, to me they want to be bytes subclasses (for many of the reasons John originally pointed out), but practically speaking, it may be easier and less disruptive to users to make them strings. We'll see! - I have a very nice PYPORTING.rst file which describes lots of the issues porting code in general, and dbus-python in particular. - make check doesn't run to successful completion for me, even from the git head <https://bugs.freedesktop.org/show_bug.cgi?id=43303>. This is on Ubuntu 11.10. I still haven't tracked this down yet. I'm fairly motivated to see a dbus-python ported to Python 3. We're making a similar push to Python 3 and our Qt/KDE users are demanding it. I'm keeping Python 2.6 support because there are only a few wormarounds different than Python 2.7, but we only need 2.7 and 3.2 support. I'm really not interested in Python 2 < 2.6 or Python 3 < 3.2. I'd like to know though whether Simon is philosophically opposed to integrating Python 3 support into upstream or not.
(In reply to comment #51) > - I did original make the various ascii-string types as bytes subclasses, and > while I was able to get test-standalone working, the cross-tests have had > various difficult to track down failures. "difficult to track down" - yes, this is why dbus-python isn't the answer :-P The type-guessing stuff is really subtle and can basically never work "properly"; the best it's ever going to get is "not too astonishing most of the time". > I'm going to do another branch where > they are str/unicode subclasses and see if that makes things better. > Conceptually, to me they want to be bytes subclasses (for many of the reasons > John originally pointed out), but practically speaking, it may be easier and > less disruptive to users to make them strings. I still think Signature and ObjectPath want to be (Unicode) strings that happen to be constrained to ASCII, for the same reasons that Python identifiers are (Unicode) strings. (UTF8String and ByteArray should be bytes subclasses, though.) > I'm really not interested in > Python 2 < 2.6 or Python 3 < 3.2. I'd like to know though whether Simon is > philosophically opposed to integrating Python 3 support into upstream or not. I'm not philosophically opposed to Python 3 stuff getting merged, but it needs a maintainer I can trust to get it right, who isn't me. If you're volunteering, help is appreciated. I *am* philosophically opposed to an API with the same flaws and backwards-compat baggage as dbus-python's being the recommended way to get on D-Bus (we'd be better off with something resembling GDBus/GVariant, I think), but perhaps the Qt world doesn't have anything better yet, so... > I've got an experimental branch in bzr. > lp:~barry/python-dbus/py3 Like other fd.o projects, dbus-python is maintained in git. If bzr can interop with git these days, great, but I'd really appreciate a git remote I can look at if possible. Or just a series of unified diffs, git-format-patch style... (the launchpad web interface has that somewhere, right?)
(In reply to comment #51) > - Python 2.6, 2.7, and 3.2 are supported, nothing older. That sounds fine, tbh; Debian stable has 2.6, and Debian stable generally makes a reasonable benchmark for "how old do we want to go?". I recently bumped the version required in master to 2.5 anyway.
On Dec 01, 2011, at 03:59 PM, bugzilla-daemon@freedesktop.org wrote: >https://bugs.freedesktop.org/show_bug.cgi?id=26420 > >--- Comment #53 from Simon McVittie <simon.mcvittie@collabora.co.uk> 2011-12-01 07:59:41 PST --- >(In reply to comment #51) >> - Python 2.6, 2.7, and 3.2 are supported, nothing older. > >That sounds fine, tbh; Debian stable has 2.6, and Debian stable generally >makes a reasonable benchmark for "how old do we want to go?". I recently >bumped the version required in master to 2.5 anyway. Excellent, that will make life much easier. :)
On Dec 01, 2011, at 03:56 PM, bugzilla-daemon@freedesktop.org wrote: >https://bugs.freedesktop.org/show_bug.cgi?id=26420 > >--- Comment #52 from Simon McVittie <simon.mcvittie@collabora.co.uk> 2011-12-01 07:56:43 PST --- >(In reply to comment #51) >> - I did original make the various ascii-string types as bytes subclasses, and >> while I was able to get test-standalone working, the cross-tests have had >> various difficult to track down failures. > >"difficult to track down" - yes, this is why dbus-python isn't the answer :-P >The type-guessing stuff is really subtle and can basically never work >"properly"; the best it's ever going to get is "not too astonishing most of >the time". Actually, the type guessing stuff works fine afaict. The bytes/unicode problem comes in callback dispatching because of all the checks against object_path, interface, method name, etc. E.g. because in Python 3 b'foo' != 'foo' all the dispatch code has to coerce everything to the same type, or callbacks won't get called. You also have tricky issues with bus names, e.g. bus_name[:1] == ':' can't work when the bus name is a bytes. The thorny problem is that in the cross-tests, the signal callbacks don't get called. The matching code is difficult to debug, so fixing one problem takes a long time and doesn't help much with the next problem. The code's gotten pretty ugly, which is why I'm coming around to your view that signatures, object paths, etc. should be unicodes. I'm not 100% there yet because I want to see if that results in cleaner internal code without undue porting burdens on user code. >> I'm going to do another branch where >> they are str/unicode subclasses and see if that makes things better. >> Conceptually, to me they want to be bytes subclasses (for many of the reasons >> John originally pointed out), but practically speaking, it may be easier and >> less disruptive to users to make them strings. > >I still think Signature and ObjectPath want to be (Unicode) strings that >happen to be constrained to ASCII, for the same reasons that Python >identifiers are (Unicode) strings. Yep, I'm going to try that approach next. >(UTF8String and ByteArray should be bytes subclasses, though.) Should UTF8String be removed for Python 3? I agree about ByteArrays. Note too that I'm planning on keeping the current long/int hierarchy when compiled against Python 2. It's only under Python 3 that things will be a little different (as minimally so as possible). >> I'm really not interested in >> Python 2 < 2.6 or Python 3 < 3.2. I'd like to know though whether Simon is >> philosophically opposed to integrating Python 3 support into upstream or not. > >I'm not philosophically opposed to Python 3 stuff getting merged, but it >needs a maintainer I can trust to get it right, who isn't me. If you're >volunteering, help is appreciated. Well, let's see if I can get the test suite passing first (see bug 43303), but if I can, I'm willing to help maintain the Python 3 stuff. >I *am* philosophically opposed to an API with the same flaws and >backwards-compat baggage as dbus-python's being the recommended way to get on >D-Bus (we'd be better off with something resembling GDBus/GVariant, I think), >but perhaps the Qt world doesn't have anything better yet, so... AFAICT, they don't. My goal with this work is to maintain as close to backward compatible API as possible for Python 3, since that would be easiest for users porting existing applications to Python 3. I don't see much of an advantage to porting dbus-python to Python 3 *and* radically changing the API because then users might as well switch to GDBus and work with the KDE and Gnome communities to improve their toolkit-specific versions. (What about non-UI dbus applications?) I guess I'm looking for guidance here as to whether that goal is compatible with your plans for upstream. Ideally, your answer would be "yes" :). If not, then I have to decide whether to abandon my work or fork upstream, neither of which are very appealing to me. >> I've got an experimental branch in bzr. >> lp:~barry/python-dbus/py3 > >Like other fd.o projects, dbus-python is maintained in git. If bzr can interop >with git these days, great, but I'd really appreciate a git remote I can look >at if possible. Or just a series of unified diffs, git-format-patch style... >(the launchpad web interface has that somewhere, right?) bzr can interoperate with git with some limitations (e.g. no submodules or colocated branches, neither of which I think are in the dbus-python git). In any case, I'll make all my work available to you via git or unified diffs. Cheers.
> >(UTF8String and ByteArray should be bytes subclasses, though.) > > Should UTF8String be removed for Python 3? I agree about ByteArrays. So, the point of UTF8String was more efficient interaction with libraries that (at the C level) are always always UTF-8 - like GLib, libxml2, expat etc. - and represent this as a str-that-is-UTF-8 in their Python 2 bindings. If such libraries would never use bytes in Python 3 anyway, then yes, UTF8String (and the accompanying utf8_strings option) should cease to exist, or be stubs that just raise an exception, in a Python 3 build. > Note too that I'm planning on keeping the current long/int hierarchy when > compiled against Python 2. It's only under Python 3 that things will be a > little different (as minimally so as possible). Sounds good. Presumably in Python 3, everything "int-ish" would be a subclass of the new int that is equivalent to the old long. > I guess I'm looking for guidance here as to whether that goal is compatible > with your plans for upstream. Ideally, your answer would be "yes" :) Yeah, let's try it. The alternative is basically "it's deprecated, use something else"... > bzr can interoperate with git with some limitations (e.g. no submodules or > colocated branches, neither of which I think are in the dbus-python git). FYI, there are branches other than "master" in dbus-python.git (e.g. see the web interface <http://cgit.freedesktop.org/dbus/dbus-python> for the list), although "master" is the only branch from which releases are made. "py3k" is J5's partially-working port to Python 3 (as referred to above), which you might find useful. "purity" is an experiment in using libdbus less, to avoid some of the worse parts of its C API. The rest are all merged, I think.
Created attachment 54126 [details] [review] A new Python 3 port of dbus-python Sorry, github apparently hates me, so I can't push a branch. Here however, is the patch to make dbus-python work with Python 2.6, 2.7, and 3.2. All the tests that pass for me on trunk (i.e. not the ones that fail in bug 43303) pass on this branch in all three Python versions too. I've followed your advice of using str (i.e. unicode) for the base class of the various utf-8 string types. PYPORT.rst contains a summary of all the effects of the port on user code, and most of the decisions I've made. I'm eager for your response!
Created attachment 54294 [details] [review] Additional patch to clean some things up This is a small follow-on patch to 26420.diff which just cleans up a few idioms based on some comments to my blog post http://www.wefearchange.org/2011/12/lessons-in-porting-to-python-3.html Also, this includes a small fix for a compilation error that someone on arch linux reported, but which I can't reproduce on Ubuntu. It should at least get rid of the error but doesn't have any other effect. Apply this on top of my previous diff. I'm still trying to get github to not hate me so I can push a branch.
Comment on attachment 54126 [details] [review] A new Python 3 port of dbus-python Review of attachment 54126 [details] [review]: ----------------------------------------------------------------- ::: PY3PORT.rst @@ +38,5 @@ > + - All object reprs are unicodes. This change was made because it greatly > + simplifies the implementation and cross-compatibility with Python 3. > + - Some values which were ints are now longs. Primarily, this affects the > + type of the `variant_level` attributes. > + - There is a new `dbus._BytesBase` class, but it is unused in Python 2. I don't think this should count as user-visible; anyone relying on _-prefixed stuff in the dbus module was already wrong. @@ +46,5 @@ > + > +What do you need to do to port that to Python 3? Here are the user visible > +changes you should be aware of. Python 3.2 is the minimal required version:: > + > + - `ByteArray`s must be initialed with bytes objects, not unicodes. Use `b''` initialized @@ +50,5 @@ > + - `ByteArray`s must be initialed with bytes objects, not unicodes. Use `b''` > + literals in the constructor. This also works in Python 2, where bytes > + objects are aliases for 8-bit strings. > + - `ByteArray`s now derive from the new `dbus._BytesBase` class instead of > + `dbus._StrBase`. If you feel the need to say this, please also say: Like everything with an underscore prefix, this is considered to be an implementation detail and may change in future versions. I think the user-visible change you're actually describing is this? `ByteArray` is now a subclass of `bytes`, where it was previously a subclass of `str`. @@ +51,5 @@ > + literals in the constructor. This also works in Python 2, where bytes > + objects are aliases for 8-bit strings. > + - `ByteArray`s now derive from the new `dbus._BytesBase` class instead of > + `dbus._StrBase`. > + - `dbus.Byte` can be constructed from a 1-character byte or str object. "... or from an integer", if it still can? @@ +53,5 @@ > + - `ByteArray`s now derive from the new `dbus._BytesBase` class instead of > + `dbus._StrBase`. > + - `dbus.Byte` can be constructed from a 1-character byte or str object. > + - `dbus.UTF8String` is gone. Use `dbus.String`. > + - `dbus._IntBase` is gone. Use `dbus._LongBase`. ... but don't anyway, because these are private. @@ +58,5 @@ > + - All longs are now ints, since Python 3 has only a single int type. This > + also means that the class hierarchy for the dbus numeric types has changed > + (all derive from int in Python 3). > + - Some exception strings have changed. > + - `dbus.is_py3` is a new flag that is True when running under Python 3. This is a change to Python 2 as well, surely? But I'd have called this dbus._is_py3 and not documented it. @@ +61,5 @@ > + - Some exception strings have changed. > + - `dbus.is_py3` is a new flag that is True when running under Python 3. > + - `dbus.keywords()` is a new function that helps with writing compatible code > + under Python 2 and Python 3. It filters out `utf8_string` arguments under > + Python 3. I don't think this deserves to be public API. dbus._adjust_keywords()? @@ +84,5 @@ > +`dbus.UTF8String` (str). Also notice that there is no direct dbus equivalent > +of Python's bytes type (although dbus does have byte arrays), so I am mapping > +dbus strings to unicodes in all cases, and getting rid of `dbus.UTF8String` in > +Python 3. I've also added a `dbus._BytesBase` type which is unused in Python > +2, but which forms the base class for `dbus.ByteArray` in Python 3. Implementation detail, not intended to be something that library users ever rely on @@ +91,5 @@ > +`dbus.Signature`), bus names, interfaces, and methods are all strings. A > +previous aborted effort was made to use bytes for these, which at first blush > +makes some sense. Practicality beats purity though, and this approach tended > +to impose too many changes on user code, and caused lots of difficult to track > +down problems, so they are all str (i.e. unicode) in Python 3. I think using strings for these is the pure choice as well as the practical choice - they're conceptually strings with a syntax. @@ +125,5 @@ > +In the above page mapping basic types, you'll notice that the Python int type > +is mapped to 32-bit signed integers ('i') and the Python long type is mapped > +to 64-bit signed integers ('x'). Python 3 doesn't have this distinction, so > +that mapping is removed. Use `dbus.UInt32` if you must have 32-bit signed > +integers. You mean dbus.Int32. I think mapping 'int' to 'x' will break real-world code; many D-Bus services are not tolerant of integer type mismatches. I'd recommend continuing to map int to 'i', even though int is now larger than it used to be. In my experience, if people want 'x' they're more likely to ask for it. @@ +134,5 @@ > +Long literals in Python code are an interesting thing to have to port. Don't > +use them if you want your code to work in both Python versions. > + > +`dbus._IntBase` is removed in Python 3, you only have `dbus._LongBase`, which > +inherits from a Python 3 int (i.e. a PyLong). Implementation detail. @@ +175,5 @@ > + > +There were a few places where I noticed what might be considered bugs, > +unchecked exception conditions, or possible reference count leaks. In these > +cases, I've just fixed what I can and hopefully haven't made the situation > +worse. You're very very welcome to report bugs for this sort of thing, preferably with patches! @@ +222,5 @@ > + - Update all C extension docstrings for accuracy. > + - Should `dbus.ByteArray` - which is a subclass of `dbus._BytesBase` which is > + itself a subclass of bytes, i.e. str in Python 2 - accept UTF8 encoded > + unicode strings? Currently, no, they must be bytes in Python 3, str in > + Python 2. I think this is right. If your string is UTF-8 (without embedded NULs), it should usually be a str/unicode in Python and a 's' on D-Bus; and if what you want is genuinely ByteArray(s.encode('utf-8')), explicit is better than implicit. D-Bus requires UTF-8 in strings (as in 's') but has nothing to say about the encoding of byte arrays. ::: _dbus_bindings/bytes.c @@ +92,5 @@ > + if (!obj_as_bytes) { > + return NULL; > + } > + obj = PyLong_FromLong( > + (unsigned char)(PyBytes_AS_STRING(obj_as_bytes)[0])); As far as I can tell, this doesn't check for length 1, so Byte(u'\x12xxxxxxxxxxxxx') is misinterpreted as Byte(u'\x12') which is 0x12. It should be an error, IMO. @@ +172,5 @@ > + PyErr_SetString(PyExc_RuntimeError, "Integer outside range 0-255"); > + return NULL; > + } > + str[0] = (unsigned char)i; > + return PyUnicode_FromStringAndSize((char *)str, 1); Is it OK for tp_str to return a unicode, even in Python 2? (I thought that was what tp_unicode was for?) ::: _dbus_bindings/conn-methods.c @@ +562,4 @@ > ok = dbus_connection_get_unix_fd (self->conn, &fd); > Py_END_ALLOW_THREADS > if (!ok) Py_RETURN_NONE; > + return PyLong_FromLong(fd); Is it considered OK for (Unix) file descriptors to be long in Python 2? (For instance, does os.fdopen work?) ::: _dbus_bindings/conn.c @@ +232,4 @@ > self->has_mainloop = (mainloop != Py_None); > self->conn = NULL; > self->filters = PyList_New(0); > + self->weaklist = NULL; Is this related to the porting, or an unrelated bug fix? @@ +324,5 @@ > + !PyBytes_Check(address_or_conn) > +#endif > + ) > + { > + PyErr_SetString(PyExc_TypeError, "connection or str expected"); I tend to prefer "is it one of these possibilities?" checks to be in the form: if (first thing) { ... } else if (second thing) { ... } else { /* none of the above, usually an error */ } Putting the "none of the above" case in the middle like you have here: if (first thing) { ... } else if (!(second thing)) { /* none of the above, usually an error */ } else { /* second thing */ } guarantees that you'll have to rewrite the conditional if you add a third thing. ::: _dbus_bindings/containers.c @@ +407,5 @@ > +#ifdef PY3K > + !PyUnicode_Check(signature) > +#else > + !PyBytes_Check(signature) > +#endif I keep seeing this construct. I feel as though for as long as we remain compatible with Python 2, the dbus-bindings header ought to #define a STR_CHECK() or something, meaning "is it the thing that this version of Python calls str?" @@ +701,5 @@ > { > PyObject *key, *value; > > +#ifdef PY3K > + if (PyUnicode_CompareWithASCIIString(name, "signature")) { Similarly, I'm starting to think we could do with a locally-defined Str_CompareWithASCIIString. ::: _dbus_bindings/generic.c @@ +38,4 @@ > { > if (op == Py_EQ || op == Py_NE) { > if (self == other) { > + return PyLong_FromLong(op == Py_EQ); Just to confirm, is it OK for comparators to return long in Python 2? ::: _dbus_bindings/int.c @@ +25,5 @@ > > #include "types-internal.h" > > +#ifdef PY3K > +#define INTBASE &DBusPyLongBase_Type Should have parentheses around the RHS, to avoid non-obvious precedence... @@ +71,5 @@ > } > tuple = Py_BuildValue("(i)", PyObject_IsTrue(value) ? 1 : 0); > if (!tuple) return NULL; > +#ifdef PY3K > + self = (DBusPyLongBase_Type.tp_new)(cls, tuple, kwargs); ... then you could use INTBASE->tp_new without the conditional :-) @@ +264,4 @@ > dbus_uint16_t > dbus_py_uint16_range_check(PyObject *obj) > { > + long i = PyLong_AsLong(obj); I think dbus_py_uint16_range_check (and friends) are sometimes called on int objects in Python 2. With that in mind, is this usage OK? ::: _dbus_bindings/message-append.c @@ +166,5 @@ > } > } > > +#ifdef PY3K > +#define FROMSTRING PyUnicode_FromString Call this STR_FROMSTRING and put it in the header? @@ +209,5 @@ > + if (PyLong_Check(obj)) { > + if (DBusPyInt64_Check(obj)) > + return PyUnicode_FromString(DBUS_TYPE_INT64_AS_STRING); > + else if (DBusPyUInt32_Check(obj)) > + return PyUnicode_FromString(DBUS_TYPE_UINT32_AS_STRING); This ordering is really weird. Any particular reason why they're not in some sort of systematic ordering, like "sort by size and then by signedness"? @@ +223,5 @@ > + return PyUnicode_FromString(DBUS_TYPE_UINT16_AS_STRING); > + else if (DBusPyBoolean_Check(obj)) > + return PyUnicode_FromString(DBUS_TYPE_BOOLEAN_AS_STRING); > + else > + return PyUnicode_FromString(DBUS_TYPE_INT64_AS_STRING); As pointed out already, I think this will break existing code, and int32 is a better default. You could make it emit deprecation warnings if you like. @@ +503,2 @@ > DBG("Message_guess_signature: returning Signature at %p \"%s\"", ret, > + ret ? PyUnicode_AS_DATA(ret) : "(NULL)"); Is %s really right to printf the array of wide characters (codepoints or UTF-16 2-byte words) in a Unicode? (I wouldn't necessarily object to just getting rid of the DBG(), though.) @@ +566,3 @@ > DBG("Performing actual append: string (from unicode) %s", s); > if (!dbus_message_iter_append_basic(appender, sig_type, &s)) { > + Py_CLEAR(utf8); This is an unrelated leak fix, right? @@ +593,5 @@ > } > + y = *(unsigned char *)PyBytes_AS_STRING(obj); > + } > + else if (PyUnicode_Check(obj)) { > + PyObject *obj_as_bytes = PyUnicode_AsUTF8String(obj); This logic (to make a Byte from a single-byte unicode string) seems really strange, and a bit inefficient. If you're using UTF-8 (but why is UTF-8 special here?), then it'd be way more efficient to check that the length of the Unicode is 1 and the first (and only) character is < 128. (Otherwise, it'd encode to more than one UTF-8 byte.) But I think more sensible semantics would be to check that the length of the Unicode is 1, and use the numeric value of the first *codepoint* - so it's an error if it isn't in the latin-1 range, between U+0000 and U+00FF? (Optimization: even if Python is encoding its strings in UTF-16 like it does on Windows, it's enough to get the first-and-only Py_UNICODE - either a UCS-4 32-bit codepoint, or a 16-bit unit of UTF-16 which is either itself or half of a surrogate pair - and check that it's in the range 0 to 255. If it is, the right answer is that byte; if not, error.) ::: _dbus_bindings/message-get-args.c @@ +252,4 @@ > args, kwargs); > } > else { > +#endif I'd prefer to still have a { in the Python 3 case so that the } doesn't need to be conditional. @@ +256,5 @@ > + /* Watch out for this call failing. */ > + unicode = PyUnicode_DecodeUTF8(u.s, strlen(u.s), NULL); > + if (!unicode) > + break; > + args = Py_BuildValue("(N)", unicode); Unrelated bugfix? Is unicode leaked here, in your version? libdbus guarantees to return valid UTF-8, and its definition of valid UTF-8 is stricter than Python's (it excludes noncharacters like U+FEFF), so this should never fail in any case. ::: _dbus_bindings/message.c @@ +115,5 @@ > return 0; > } > > +static PyObject * > +MethodCallMessage_tp_repr(PyObject *self) Not really related to this port... @@ +133,5 @@ > + if (!destination) > + destination = "n/a"; > + > + return PyUnicode_FromFormat( > + "<%s path: %s, int: %s, member: %s dest: %s>", In D-Bus we usually say "iface" if "interface" would be too long. ::: _dbus_bindings/server.c @@ +101,4 @@ > { > const char *list[length + 1]; > > + if (!(references = PyTuple_New(length))) Is it OK to free a tuple some of whose entries are NULL? @@ +402,4 @@ > > self = DBusPyServer_NewConsumingDBusServer(cls, server, conn_class, > mainloop, auth_mechanisms); > + ((Server *)self)->weaklist = NULL; Unrelated fix, or what? ::: configure.ac @@ +164,4 @@ > nested-externs \ > pointer-arith \ > format-security \ > + no-unused-parameter \ This is in the wrong place; put "unused-parameter" in the list of undesirable warnings, next to "missing-field-initializers", without the "no-" prefix. (The difference is that for each thing in the list of undesirable warnings, if "-Wno-THING -Wno-error=THING" doesn't work, the macro will refuse to enable -Werror.) ::: dbus/__init__.py @@ +68,5 @@ > # submodules > 'service', 'mainloop', 'lowlevel' > + ] > + > +is_py3 = getattr(sys.version_info, 'major', sys.version_info[0]) == 3 Should be private, IMO... or if not, should be documented, and questions should be asked about why Python itself doesn't provide this. (Shouldn't it be >=, too?) Having this in 'dbus' (as opposed to dbus._compat or something) works against the possibility of ever not having this module be full of circular dependencies... although we can probably never break the circular dependencies without breaking API anyway. @@ +73,5 @@ > + > +if not is_py3: > + __all__.append('UTF8String') > + > +def keywords(**kws): Should be private, IMO. I think it should also raise an exception if you use utf8_strings (at least if it's set to a true value) in Python 3, rather than silently swallowing it - then part of porting to Python 3 will be "stop using utf8_strings, which no longer does anything useful". ::: dbus/connection.py @@ +28,2 @@ > try: > + import _thread as thread Is this really the recommended way to do things in Python-3-land? Aren't underscore prefixes normally meant to mean "no user-serviceable parts inside"? @@ +186,5 @@ > return False > if self._int_args_match is not None: > # extracting args with utf8_strings and byte_arrays is less work > + args = message.get_args_list( > + **keywords(utf8_strings=True, byte_arrays=True)) It seems as though now it'd be better to just accept the overhead of UTF-8 -> Unicode conversion... @@ +192,4 @@ > if (index >= len(args) > + or (not isinstance(args[index], UTF8String) > + if not is_py3 > + else False) ... which would let you use String as the thing that you check with isinstance, which can then be unconditional. Removing the UTF8String check is a semantic change: previously we insisted that arguments matched with args_match were (in D-Bus terms) strings, whereas now you'll accept anything string-like. The underlying D-Bus match rules only accept strings (and not, for instance, object paths), so we should be consistent with that. @@ +210,5 @@ > # doing so again > if args is None or not self._utf8_strings or not self._byte_arrays: > + args = message.get_args_list( > + **keywords(utf8_strings=self._utf8_strings, > + byte_arrays=self._byte_arrays)) Be careful to change this too, when changing the above. ::: dbus/dbus_bindings.py @@ +23,1 @@ > from dbus.types import * dbus_bindings is so deprecated that it should never have existed; it only exists to support apps that were wrongly using the C-level part of the binding (what's now _dbus_bindings). Can we take the opportunity to just not install it, or have it raise an exception on import, for Python 3?
Comment on attachment 54294 [details] [review] Additional patch to clean some things up Review of attachment 54294 [details] [review]: ----------------------------------------------------------------- These changes to the earlier patch look fine.
So the main thing I'm not happy about here is the handling of UTF8String. I've already explained what the rationale for UTF8String was, and it makes no sense in a Python 3 world, so I think we can categorise it as "failed experiment" and move on. Here is my suggestion: * Don't make keywords() public or expect library users to call it * Don't use UTF8String in library code except where needed to implement existing semantics; don't use it in tests except where the purpose of the test is to demonstrate that UTF8String works * Skip tests whose only purpose is to demonstrate UTF8String in Python 3, since they're meaningless
(In reply to comment #61) > I've already explained what the rationale for UTF8String was, and it makes no > sense in a Python 3 world, so I think we can categorise it as "failed > experiment" and move on ... meaning that for dbus-python users wanting to be portable to Python 3, step 1 is "stop using utf8_strings". (It's not going to work the way it was intended to work in Python 3 *anyway*, so they need to deal with that.)
Thanks for the review Simon. For the sake of brevity, I'll omit any comments for issues that I'll just go ahead and fix. Would you rather see a patch that addresses all your comments, and which obsoletes the previous patches, or would you rather see one an incremental patch to apply on top of the ones you've already reviewed? (I'm still trying to get github to not hate me. ;) On Dec 12, 2011, at 12:28 PM, bugzilla-daemon@freedesktop.org wrote: >::: PY3PORT.rst >@@ +38,5 @@ >> + - All object reprs are unicodes. This change was made because it greatly >> + simplifies the implementation and cross-compatibility with Python 3. >> + - Some values which were ints are now longs. Primarily, this affects the >> + type of the `variant_level` attributes. >> + - There is a new `dbus._BytesBase` class, but it is unused in Python 2. > >I don't think this should count as user-visible; anyone relying on _-prefixed >stuff in the dbus module was already wrong. Sounds right, I'll remove this from "user-visible changes". Since the class is unused in Python 2, should I #ifdef it out when compiled under Python 2? >@@ +50,5 @@ >> + - `ByteArray`s must be initialed with bytes objects, not unicodes. Use `b''` >> + literals in the constructor. This also works in Python 2, where bytes >> + objects are aliases for 8-bit strings. >> + - `ByteArray`s now derive from the new `dbus._BytesBase` class instead of >> + `dbus._StrBase`. > >If you feel the need to say this, please also say: Like everything with an >underscore prefix, this is considered to be an implementation detail and may >change in future versions. I've replaced this with... > >I think the user-visible change you're actually describing is this? > >`ByteArray` is now a subclass of `bytes`, where it was previously a subclass >of `str`. - `ByteArray` is now a subclass of `bytes`, where in Python 2 it is a subclass of `str`. >@@ +51,5 @@ >> + literals in the constructor. This also works in Python 2, where bytes >> + objects are aliases for 8-bit strings. >> + - `ByteArray`s now derive from the new `dbus._BytesBase` class instead of >> + `dbus._StrBase`. >> + - `dbus.Byte` can be constructed from a 1-character byte or str object. > >"... or from an integer", if it still can? It can. Fixed description. >@@ +53,5 @@ >> + - `ByteArray`s now derive from the new `dbus._BytesBase` class instead of >> + `dbus._StrBase`. >> + - `dbus.Byte` can be constructed from a 1-character byte or str object. >> + - `dbus.UTF8String` is gone. Use `dbus.String`. >> + - `dbus._IntBase` is gone. Use `dbus._LongBase`. > >... but don't anyway, because these are private. I'll just remove this. > >@@ +58,5 @@ >> + - All longs are now ints, since Python 3 has only a single int type. This >> + also means that the class hierarchy for the dbus numeric types has changed >> + (all derive from int in Python 3). >> + - Some exception strings have changed. >> + - `dbus.is_py3` is a new flag that is True when running under Python 3. > >This is a change to Python 2 as well, surely? But I'd have called this >dbus._is_py3 and not documented it. Fair enough. There's nothing so magical about that variable that it needs to be in the public API. In which case, I can remove this item since it's not a user-visible change. >@@ +61,5 @@ >> + - Some exception strings have changed. >> + - `dbus.is_py3` is a new flag that is True when running under Python 3. >> + - `dbus.keywords()` is a new function that helps with writing compatible code >> + under Python 2 and Python 3. It filters out `utf8_string` arguments under >> + Python 3. > >I don't think this deserves to be public API. dbus._adjust_keywords()? Yes. Let's also suggest people just stop using `utf8_strings`. >I think mapping 'int' to 'x' will break real-world code; many D-Bus services >are not tolerant of integer type mismatches. I'd recommend continuing to map >int to 'i', even though int is now larger than it used to be. In my >experience, if people want 'x' they're more likely to ask for it. So, specifically, that message Message.guess_signature(7) should return dbus.Signature('i') instead of dbus.Signature('x'). Are there other places that need fixing? What would (or should) happen if an interface specified to use 'i' is handed a Python integer outside that range? Ideally, an exception would be thrown instead of data corruption. It's not clear to me that this is handled, or where the int value would be checked, or if there are unit tests for this case. Any thoughts? There did not seem to be a test in the test suite either for the specific case of guessing 'i' for ints, so I added one. After making this change, I see no other impact on the test suite, so it's not clear if there's anything else I need to change. The new test is in test-standalone.py: def test_guess_signature_python_ints(self): aeq = self.assertEqual from _dbus_bindings import Message aeq(Message.guess_signature(7), 'i') if not _is_py3: aeq(Message.guess_signature(make_long(7)), 'x') >@@ +175,5 @@ >> + >> +There were a few places where I noticed what might be considered bugs, >> +unchecked exception conditions, or possible reference count leaks. In these >> +cases, I've just fixed what I can and hopefully haven't made the situation >> +worse. > >You're very very welcome to report bugs for this sort of thing, preferably >with patches! Mostly, I just folded them into this mega-patch, or rejiggered the implementation enough to replace the code. The one refleak that comes to mind is in DBusPyServer_set_auth_mechanisms(). PySequence_Fast() returns a new reference, but it's never decref'd at the end of the function. Not to worry though because when I adjusted the implementation to handle unicodes coming out of auth_mechanisms, I had to free the intermediate bytes objects at the end anyway, so I added a decref of fast_seq. >@@ +222,5 @@ >> + - Update all C extension docstrings for accuracy. >> + - Should `dbus.ByteArray` - which is a subclass of `dbus._BytesBase` which is >> + itself a subclass of bytes, i.e. str in Python 2 - accept UTF8 encoded >> + unicode strings? Currently, no, they must be bytes in Python 3, str in >> + Python 2. > >I think this is right. If your string is UTF-8 (without embedded NULs), it >should usually be a str/unicode in Python and a 's' on D-Bus; and if what you >want is genuinely ByteArray(s.encode('utf-8')), explicit is better than >implicit. I agree! I'll remove this as an open issue. >D-Bus requires UTF-8 in strings (as in 's') but has nothing to say about the >encoding of byte arrays. > >::: _dbus_bindings/bytes.c >@@ +92,5 @@ >> + if (!obj_as_bytes) { >> + return NULL; >> + } >> + obj = PyLong_FromLong( >> + (unsigned char)(PyBytes_AS_STRING(obj_as_bytes)[0])); > >As far as I can tell, this doesn't check for length 1, so > > Byte(u'\x12xxxxxxxxxxxxx') > >is misinterpreted as > > Byte(u'\x12') > >which is 0x12. It should be an error, IMO. Good catch, thanks. I'll add a test for this and fix it. > >@@ +172,5 @@ >> + PyErr_SetString(PyExc_RuntimeError, "Integer outside range 0-255"); >> + return NULL; >> + } >> + str[0] = (unsigned char)i; >> + return PyUnicode_FromStringAndSize((char *)str, 1); > >Is it OK for tp_str to return a unicode, even in Python 2? (I thought that was >what tp_unicode was for?) Yes. Despite the documentation, in both Python 2.6 and 2.7, it's legal to return unicodes from tp_str and tp_repr, as well as __str__() and __repr__(). I actually looked at the Python source code to figure out exactly what happens. When one of these returns a unicode, it gets turned into an 8-bit string via PyUnicode_AsEncodedString() with NULL error and encoding arguments. This means the default encoding will be used, and as shown by sys.getdefaultencoding() will always be 'ascii' unless someone (e.g. the OS vendor) hacks site.py to do something different. Debian/Ubuntu doesn't change this near as I can tell, and while I can't speak for other distros, I think they'd be insane to do anything different. ;) This make sense, exactly for Python 3 portability, though it probably should be documented. So even though we can return unicodes from tp_str in both cases, a Python user will always see strs in both cases. E.g. 8-bit strings in Python 2 and unicodes in Python 3. Since dbus-python's reprs and strs should always be ascii encoded, this should always be safe. >::: _dbus_bindings/conn-methods.c >@@ +562,4 @@ >> ok = dbus_connection_get_unix_fd (self->conn, &fd); >> Py_END_ALLOW_THREADS >> if (!ok) Py_RETURN_NONE; >> + return PyLong_FromLong(fd); > >Is it considered OK for (Unix) file descriptors to be long in Python 2? (For >instance, does os.fdopen work?) Yep. I didn't dig into the code, but here's what happens on Ubuntu: Python 2.6.7 (r267:88850, Aug 11 2011, 12:18:09) [GCC 4.6.1] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> import os >>> fp = os.fdopen(2L) >>> fp.fileno() 2 You'll get an OverflowError if the long can't be converted to an int. >::: _dbus_bindings/conn.c >@@ +232,4 @@ >> self->has_mainloop = (mainloop != Py_None); >> self->conn = NULL; >> self->filters = PyList_New(0); >> + self->weaklist = NULL; > >Is this related to the porting, or an unrelated bug fix? It's related to the porting, as (one of) the porting guides recommendations for handling weaklists in Python 3. Py_TPFLAGS_HAVE_WEAKREFS is gone. It may be overkill but it can't hurt. >@@ +324,5 @@ >> + !PyBytes_Check(address_or_conn) >> +#endif >> + ) >> + { >> + PyErr_SetString(PyExc_TypeError, "connection or str expected"); > >I tend to prefer "is it one of these possibilities?" checks to be in the form: > > if (first thing) { > ... > } > else if (second thing) { > ... > } > else { > /* none of the above, usually an error */ > } > >Putting the "none of the above" case in the middle like you have here: > > if (first thing) { > ... > } > else if (!(second thing)) { > /* none of the above, usually an error */ > } > else { > /* second thing */ > } > >guarantees that you'll have to rewrite the conditional if you add a third >thing. Fair enough. I definitely want to stick to your preferred style, although it does open us up for some code duplication; removing that duplication doesn't seem worth complicating the implementation. In this particular case, is there a reason not to accept either bytes or unicodes in both versions of Python? My rewrite based on your feedback does enable that, but if you'd rather I can add some #ifdefs to limit it to "native strings" (see below). >::: _dbus_bindings/containers.c >@@ +407,5 @@ >> +#ifdef PY3K >> + !PyUnicode_Check(signature) >> +#else >> + !PyBytes_Check(signature) >> +#endif > >I keep seeing this construct. I feel as though for as long as we remain >compatible with Python 2, the dbus-bindings header ought to #define a >STR_CHECK() or something, meaning "is it the thing that this version of Python >calls str?" Alternatively, we could accept both bytes and unicodes for these APIs (see above). But if you prefer to accept only "native strings", i.e. whatever that version of Python calls `str`, then the macro you suggest makes sense. Unlike above, in this case, I added the macro (NATIVE_STR_CHECK) and limited the implementation, but it's probably better to be consistent. I don't have a strong feeling either way, so let me know what you'd prefer and I'll fix all the cases. >@@ +701,5 @@ >> { >> PyObject *key, *value; >> >> +#ifdef PY3K >> + if (PyUnicode_CompareWithASCIIString(name, "signature")) { > >Similarly, I'm starting to think we could do with a locally-defined >Str_CompareWithASCIIString. I think this case is a bit different, because you're not going to get bytes attribute names in Python 3, so that version has a much shorter implementation. The existing Python 2 code allows 8-bit or unicode attribute names, coerces the latter to 8-bit strings and then does a strcmp() for the comparison. So I think the utility of the macro depends on whether you want to keep that behavior for Python 2 or not. If so, then the macro doesn't buy you much I think. I.e. it's probably not worth putting all that coercion and error checking into the macro, because the error-exit cleanups could potentially be different. >::: _dbus_bindings/generic.c >@@ +38,4 @@ >> { >> if (op == Py_EQ || op == Py_NE) { >> if (self == other) { >> + return PyLong_FromLong(op == Py_EQ); > >Just to confirm, is it OK for comparators to return long in Python 2? I went to the Python source to double check, but after calling the tp_richcompare slot function, Python turns this into a C int via PyObject_IsTrue(). Other than checking against Py_NotImplemented, the rich comparison's return value type isn't checked. OTOH, unless I'm mistaken, dbus_py_tp_richcompare_by_pointer() and dbus_py_tp_hash_by_pointer() aren't used anywhere internally. Is it worth even keeping these? (or does my grep-fu suck? :) >::: _dbus_bindings/int.c >@@ +25,5 @@ >> >> #include "types-internal.h" >> >> +#ifdef PY3K >> +#define INTBASE &DBusPyLongBase_Type > >Should have parentheses around the RHS, to avoid non-obvious precedence... Good catch, thanks. > >@@ +71,5 @@ >> } >> tuple = Py_BuildValue("(i)", PyObject_IsTrue(value) ? 1 : 0); >> if (!tuple) return NULL; >> +#ifdef PY3K >> + self = (DBusPyLongBase_Type.tp_new)(cls, tuple, kwargs); > >... then you could use INTBASE->tp_new without the conditional :-) Right! :) >@@ +264,4 @@ >> dbus_uint16_t >> dbus_py_uint16_range_check(PyObject *obj) >> { >> + long i = PyLong_AsLong(obj); > >I think dbus_py_uint16_range_check (and friends) are sometimes called on int >objects in Python 2. With that in mind, is this usage OK? Yep. Again U(ing)TSL shows that PyLong_AsLong() drops back to PyInt_AsLong() in Python 2. >::: _dbus_bindings/message-append.c >@@ +166,5 @@ >> } >> } >> >> +#ifdef PY3K >> +#define FROMSTRING PyUnicode_FromString > >Call this STR_FROMSTRING and put it in the header? Let's go with NATIVE_FROMSTR to match the previous macro. >@@ +209,5 @@ >> + if (PyLong_Check(obj)) { >> + if (DBusPyInt64_Check(obj)) >> + return PyUnicode_FromString(DBUS_TYPE_INT64_AS_STRING); >> + else if (DBusPyUInt32_Check(obj)) >> + return PyUnicode_FromString(DBUS_TYPE_UINT32_AS_STRING); > >This ordering is really weird. Any particular reason why they're not in some >sort of systematic ordering, like "sort by size and then by signedness"? Mostly because I copied the ordering from the Python 2 PyInt and PyLong cases and then tried to combine them. I noticed the comment just before this stanza: /* Ordering is important: some of these are subclasses of each other. */ so I wanted to mess with that ordering as little as possible. Maybe it does make more sense to change the ordering under Python 3 as you suggest, since there's only one that covers all int types. What do you think? >@@ +223,5 @@ >> + return PyUnicode_FromString(DBUS_TYPE_UINT16_AS_STRING); >> + else if (DBusPyBoolean_Check(obj)) >> + return PyUnicode_FromString(DBUS_TYPE_BOOLEAN_AS_STRING); >> + else >> + return PyUnicode_FromString(DBUS_TYPE_INT64_AS_STRING); > >As pointed out already, I think this will break existing code, and int32 is a >better default. You could make it emit deprecation warnings if you like. I think I'm happy just to make it return INT32. By emitting deprecation warnings, I think that's similar to your previous suggestion that the type inferencing shouldn't be relied upon, and probably deprecated. Hmm, it might not be a bad idea, but I don't have strong opinions about it either way. >@@ +503,2 @@ >> DBG("Message_guess_signature: returning Signature at %p \"%s\"", ret, >> + ret ? PyUnicode_AS_DATA(ret) : "(NULL)"); > >Is %s really right to printf the array of wide characters (codepoints or >UTF-16 2-byte words) in a Unicode? > >(I wouldn't necessarily object to just getting rid of the DBG(), though.) PyUnicode_AS_DATA() returns a char*, but it is the internal buffer, so while probably safe, I guess it might not really be the right thing to return. I was lazy and didn't want to add all the conversion stuff for a DBG message. Yeah, let's just remove the DBG. >@@ +566,3 @@ >> DBG("Performing actual append: string (from unicode) %s", s); >> if (!dbus_message_iter_append_basic(appender, sig_type, &s)) { >> + Py_CLEAR(utf8); > >This is an unrelated leak fix, right? Yep. >@@ +593,5 @@ >> } >> + y = *(unsigned char *)PyBytes_AS_STRING(obj); >> + } >> + else if (PyUnicode_Check(obj)) { >> + PyObject *obj_as_bytes = PyUnicode_AsUTF8String(obj); > >This logic (to make a Byte from a single-byte unicode string) seems really >strange, and a bit inefficient. > >If you're using UTF-8 (but why is UTF-8 special here?), then it'd be way more >efficient to check that the length of the Unicode is 1 and the first (and >only) character is < 128. (Otherwise, it'd encode to more than one UTF-8 >byte.) > >But I think more sensible semantics would be to check that the length of the >Unicode is 1, and use the numeric value of the first *codepoint* - so it's an >error if it isn't in the latin-1 range, between U+0000 and U+00FF? > >(Optimization: even if Python is encoding its strings in UTF-16 like it does >on Windows, it's enough to get the first-and-only Py_UNICODE - either a UCS-4 >32-bit codepoint, or a 16-bit unit of UTF-16 which is either itself or half >of a surrogate pair - and check that it's in the range 0 to 255. If it is, >the right answer is that byte; if not, error.) XXX COME BACK TO ME >::: _dbus_bindings/message-get-args.c >@@ +252,4 @@ >> args, kwargs); >> } >> else { >> +#endif > >I'd prefer to still have a { in the Python 3 case so that the } doesn't need to >be conditional. > >@@ +256,5 @@ >> + /* Watch out for this call failing. */ >> + unicode = PyUnicode_DecodeUTF8(u.s, strlen(u.s), NULL); >> + if (!unicode) >> + break; >> + args = Py_BuildValue("(N)", unicode); > >Unrelated bugfix? Yep. >Is unicode leaked here, in your version? It's not, because the 'N' code is just like 'O' except it steals the reference. (Technically, it doesn't incref the argument, so it steals it even if the call fails for other reasons.) >libdbus guarantees to return valid UTF-8, and its definition of valid UTF-8 is >stricter than Python's (it excludes noncharacters like U+FEFF), so this should >never fail in any case. Cool. This is mostly just my hyper-cautious Python C API hacker trait showing through. :) >::: _dbus_bindings/message.c >@@ +115,5 @@ >> return 0; >> } >> >> +static PyObject * >> +MethodCallMessage_tp_repr(PyObject *self) > >Not really related to this port... No, but I found it *really* helpful for debugging it. >::: _dbus_bindings/server.c >@@ +101,4 @@ >> { >> const char *list[length + 1]; >> >> + if (!(references = PyTuple_New(length))) > >Is it OK to free a tuple some of whose entries are NULL? Yep, it's a pretty typical Python idiom. >@@ +402,4 @@ >> >> self = DBusPyServer_NewConsumingDBusServer(cls, server, conn_class, >> mainloop, auth_mechanisms); >> + ((Server *)self)->weaklist = NULL; > >Unrelated fix, or what? Yep, see the above discussion about the weakref differences in Python 3. >::: configure.ac >@@ +164,4 @@ >> nested-externs \ >> pointer-arith \ >> format-security \ >> + no-unused-parameter \ > >This is in the wrong place; put "unused-parameter" in the list of undesirable >warnings, next to "missing-field-initializers", without the "no-" prefix. > >(The difference is that for each thing in the list of undesirable warnings, if >"-Wno-THING -Wno-error=THING" doesn't work, the macro will refuse to enable >-Werror.) Gotcha, thanks. I should have been more careful about checking that macro. I fixed the upstream Python bug that forces this on us, but it won't be available until Python 3.2.3 (or Python 3.3). Still, we probably want to keep it even once that version's been released, so it'll still compile with 3.2, 3.2.1, and 3.2.2. >::: dbus/__init__.py >@@ +68,5 @@ >> # submodules >> 'service', 'mainloop', 'lowlevel' >> + ] >> + >> +is_py3 = getattr(sys.version_info, 'major', sys.version_info[0]) == 3 > >Should be private, IMO... or if not, should be documented, and questions >should be asked about why Python itself doesn't provide this. Yep, I made it private. Python doesn't provide this because it's mostly a convenience to make dbus-python simpler. >(Shouldn't it be >=, too?) It could be, but hopefully we'll all be long retired before Python 4 shows up. :) >Having this in 'dbus' (as opposed to dbus._compat or something) works against >the possibility of ever not having this module be full of circular >dependencies... although we can probably never break the circular dependencies >without breaking API anyway. That's a good point. I did think about the circular imports problem here, which is why it's sometimes imported at function scope instead of module scope. I toyed with the idea of a _compat module for this and keywords() but eventually didn't need it. I'm happy to move those helpers to a dbus._compat if you prefer. >@@ +73,5 @@ >> + >> +if not is_py3: >> + __all__.append('UTF8String') >> + >> +def keywords(**kws): > >Should be private, IMO. Yep, done as part of your previous suggestion. >I think it should also raise an exception if you use utf8_strings (at least if >it's set to a true value) in Python 3, rather than silently swallowing it - >then part of porting to Python 3 will be "stop using utf8_strings, which no >longer does anything useful". The one reason why it silently swallows the argument is to aid in porting the existing uses of utf8_strings in the Python layer of the library. For example, in bus.py, you'll find this in methods such as .list_names(), .get_name_owner() and so on. You'll find its use in other places in the library, and in the test suite. I wanted to simplify the cross-Python porting of these internal implementation and test methods; I wasn't as much concerned about user code. Raising an exception will make it more difficult to have one code construct for both versions. I think making this a private function will mean that users should never use it and thus not get much benefit out of an exception. I'm fairly certain that at the C layer, nothing accepts a `utf8_strings` argument in Python 3. E.g. under Python 3, it's #ifdef'd out of the Message_get_args_options struct and the acceptable keyword arguments in dbus_py_Message_get_args_list(). It's not accepted as a positional argument in the latter under Python 3 either. It gets a little trickier at the Python layer. For example, there's a slot in SignalMatch for _utf8_strings, but it's essentially ignored. add_signal_receiver() also accepts but ignores it, as does call_async() and call_blocking() in connection.py. There may be other cases. The question is whether we want to track these down and add exceptions or deprecation warnings to nudge folks into getting rid of these (and in some cases, what to do about their use within the dbus-python package itself). Is it worth it to complicate the Python code, or break backward compatibility? I couldn't answer these questions, so I just punted and ignored it, but I'm happy to do something different, based on your answers to the above. >::: dbus/connection.py >@@ +28,2 @@ >> try: >> + import _thread as thread > >Is this really the recommended way to do things in Python-3-land? Aren't >underscore prefixes normally meant to mean "no user-serviceable parts inside"? Yes, but this is essentially the transformation that 2to3 would do, so I mimicked that with my one-code-base approach. See the Note at the top of http://docs.python.org/library/thread.html for "official" recommendations here. I looked into this in more detail, and AFAICT, connection.py and service.py really only wants the thread module for .allocate_lock(), so this one could easily be changed to use threading.Lock (or RLock?) objects. One thing I could not answer though is why _dbus.py imports thread. Maybe we can get rid of that and switch the other two to use threading? >@@ +186,5 @@ >> return False >> if self._int_args_match is not None: >> # extracting args with utf8_strings and byte_arrays is less work >> + args = message.get_args_list( >> + **keywords(utf8_strings=True, byte_arrays=True)) > >It seems as though now it'd be better to just accept the overhead of UTF-8 -> >Unicode conversion... ... >@@ +192,4 @@ >> if (index >= len(args) >> + or (not isinstance(args[index], UTF8String) >> + if not is_py3 >> + else False) > >... which would let you use String as the thing that you check with >isinstance, which can then be unconditional. > >Removing the UTF8String check is a semantic change: previously we insisted >that arguments matched with args_match were (in D-Bus terms) strings, whereas >now you'll accept anything string-like. The underlying D-Bus match rules only >accept strings (and not, for instance, object paths), so we should be >consistent with that. Right, so the big question for you is whether that semantic and API change is acceptable. Getting rid of UTF8String and utf8_strings in both versions of Python certainly would clean things up, at the cost of breaking existing code (including dbus-python internally, which would be fixable). I opted for maximal compatibility with existing Python 2 code, but I'd certainly prefer things to be cleaner. :) I'll let you decide and will abide by your preference. >@@ +210,5 @@ >> # doing so again >> if args is None or not self._utf8_strings or not self._byte_arrays: >> + args = message.get_args_list( >> + **keywords(utf8_strings=self._utf8_strings, >> + byte_arrays=self._byte_arrays)) > >Be careful to change this too, when changing the above. > >::: dbus/dbus_bindings.py >@@ +23,1 @@ >> from dbus.types import * > >dbus_bindings is so deprecated that it should never have existed; it only >exists to support apps that were wrongly using the C-level part of the binding >(what's now _dbus_bindings). Can we take the opportunity to just not install >it, or have it raise an exception on import, for Python 3? It does emit a DeprecationWarning currently for both versions, so raising an exception (ImportError most likely) in Python 3 is an easy change. Perhaps it should just be removed altogether at this point? That's a change I'll leave to you to decide. :) I've made all the uncontroversial changes discussed above, and I'm just going to finish up the testing. I really appreciate the review, given how big the changes are. Let me know whether you'd prefer a full diff or incremental, and let me know what you think about the open questions above, and I can upload new diffs by tomorrow. Cheers, -Barry
On Dec 12, 2011, at 12:41 PM, bugzilla-daemon@freedesktop.org wrote: >So the main thing I'm not happy about here is the handling of UTF8String. > >I've already explained what the rationale for UTF8String was, and it makes no >sense in a Python 3 world, so I think we can categorise it as "failed >experiment" and move on. Here is my suggestion: Yep. I was careful (but maybe screwed up) to remove UTF8String from the Python 3 port. It still exists for backward compatibility in Python 2. >* Don't make keywords() public or expect library users to call it Done. >* Don't use UTF8String in library code except where needed to implement > existing semantics; don't use it in tests except where the purpose of > the test is to demonstrate that UTF8String works > >* Skip tests whose only purpose is to demonstrate UTF8String in Python 3, > since they're meaningless All the places where UTF8String occurs in the tests are protected by the `if not _is_py3` test, meaning those tests only run in Python 2. Note that I have not updated the docstrings to reflect this, but I have a big TODO to fix up the docstrings. Similarly I think UTF8String only barely shows up at the Python level anyway, and its import or exposure in __all__ is always conditionalized on the Python version. Was there something more you want me to do?
(In reply to comment #63) > Would you rather see a patch that addresses all your comments, and which > obsoletes the previous patches, or would you rather see one an incremental > patch to apply on top of the ones you've already reviewed? Either works. To be honest, the easiest thing to review would have been a series of "atomic" patches (e.g. "change the base types of all int subclasses to be long under Python 3"), with the least controversial/most obvious changes first - then I could start applying the stuff I liked already, and skip the bits that still had problems (or depended on something problematic). > (I'm still trying to get github to not hate me. ;) gitorious.org and freedesktop.org can also host personal git repositories, if that's any help, as do repo.or.cz and Bitbucket (although I don't have accounts on the latter two). > >I don't think this should count as user-visible; anyone relying on _-prefixed > >stuff in the dbus module was already wrong. > > Sounds right, I'll remove this from "user-visible changes". Since the class > is unused in Python 2, should I #ifdef it out when compiled under Python 2? Ideally, yes. > - `ByteArray` is now a subclass of `bytes`, where in Python 2 it is a > subclass of `str`. Looks good. > So, specifically, that message Message.guess_signature(7) should return > dbus.Signature('i') instead of dbus.Signature('x'). Are there other places > that need fixing? No, I think everything else is driven by guess_signature. > What would (or should) happen if an interface specified to use 'i' is handed a > Python integer outside that range? It raises an exception from dbus_int32_range_check(). TypeError, I think? There is no buffer overflow or whatever, but the Message is ruined and you have to start again (a limitation of DBusMessage in libdbus). > >Putting the "none of the above" case in the middle like you have here ... > >guarantees that you'll have to rewrite the conditional if you add a third > >thing. > > In this particular case, is there a reason not to accept either bytes or > unicodes in both versions of Python? My rewrite based on your feedback does > enable that, but if you'd rather I can add some #ifdefs to limit it to "native > strings" (see below). This is for signatures, right? The fact that Python 2 doesn't allow unicode signatures is arguably a bug; if you wanted to relax this, it's not a problem. Python 3 accepting bytes signatures would be a bit strange, but not really a big deal, so if you wanted to allow this, I wouldn't mind. > I think this case is a bit different, because you're not going to get bytes > attribute names in Python 3, so that version has a much shorter > implementation. The existing Python 2 code allows 8-bit or unicode attribute > names, coerces the latter to 8-bit strings and then does a strcmp() for the > comparison. So I think the utility of the macro depends on whether you want > to keep that behavior for Python 2 or not. If so, then the macro doesn't buy > you much I think. OK, leave it. > OTOH, unless I'm mistaken, dbus_py_tp_richcompare_by_pointer() and > dbus_py_tp_hash_by_pointer() aren't used anywhere internally. Is it worth > even keeping these? (or does my grep-fu suck? :) If we don't need them, feel free to delete them. > >This ordering is really weird. Any particular reason why they're not in some > >sort of systematic ordering, like "sort by size and then by signedness"? > > Mostly because I copied the ordering from the Python 2 PyInt and PyLong cases > and then tried to combine them. I noticed the comment just before this > stanza: > > /* Ordering is important: some of these are subclasses of each other. */ > > so I wanted to mess with that ordering as little as possible. Right, the ordering that comment refers to is (for instance) that the cases for Int64 and UInt64 have to come before the case for plain 'long', and the cases for ObjectPath and Signature have to come before the case for plain 'str' (whatever that means in the current version). None of the user-visible dbus-python types (Int64 etc.) are subclasses of each other, so it's safe to re-order those. > >@@ +566,3 @@ > >> DBG("Performing actual append: string (from unicode) %s", s); > >> if (!dbus_message_iter_append_basic(appender, sig_type, &s)) { > >> + Py_CLEAR(utf8); > > > >This is an unrelated leak fix, right? > > Yep. I'll apply that prior to the Python 3 stuff. > >This logic (to make a Byte from a single-byte unicode string) seems really > >strange, and a bit inefficient. > > XXX COME BACK TO ME Still to be looked at > >@@ +256,5 @@ > >> + /* Watch out for this call failing. */ > >> + unicode = PyUnicode_DecodeUTF8(u.s, strlen(u.s), NULL); > >> + if (!unicode) > >> + break; > >> + args = Py_BuildValue("(N)", unicode); > > > >Unrelated bugfix? > > Yep. I should apply that before merging the Python 3 stuff, too. > >> +static PyObject * > >> +MethodCallMessage_tp_repr(PyObject *self) > > > >Not really related to this port... > > No, but I found it *really* helpful for debugging it. OK, I can probably pull this in as a separate commit too. > The one reason why it silently swallows the argument is to aid in porting the > existing uses of utf8_strings in the Python layer of the library. Yes... but I feel as though accepting an argument that doesn't do what it's claimed to is rather dishonest. > It gets a little trickier at the Python layer. For example, there's a slot in > SignalMatch for _utf8_strings, but it's essentially ignored. > add_signal_receiver() also accepts but ignores it, as does call_async() and > call_blocking() in connection.py. There may be other cases. The question is > whether we want to track these down and add exceptions or deprecation warnings > to nudge folks into getting rid of these (and in some cases, what to do about > their use within the dbus-python package itself). Is it worth it to > complicate the Python code, or break backward compatibility? I think grepping for utf8_strings should be enough to fix the internal uses? Some cases will probably have to turn from foo(utf8_strings=utf8_strings) into kwargs = {} if is_py3 and utf8_strings: kwargs['utf8_strings'] = True foo(**kwargs) but I think that's a necessary evil. Fundamentally, anyone who's relying on utf8_strings behaviour under Python 3 has already lost, because they can't have it. > I looked into this in more detail, and AFAICT, connection.py and service.py > really only wants the thread module for .allocate_lock(), so this one could > easily be changed to use threading.Lock (or RLock?) objects. > > One thing I could not answer though is why _dbus.py imports thread. Maybe we > can get rid of that and switch the other two to use threading? You know Python threads better than I do, tbh... > >Removing the UTF8String check is a semantic change: previously we insisted > >that arguments matched with args_match were (in D-Bus terms) strings, whereas > >now you'll accept anything string-like. > > Right, so the big question for you is whether that semantic and API change is > acceptable. To be clear, here is the semantic change I don't like: consider this D-Bus match rule: "arg0='/'" and these messages: o = dbus.lowlevel.SignalMessage('/', 'a.b', 'c') o.append('/', signature='o') # 0'th argument is an object path s = dbus.lowlevel.SignalMessage('/', 'a.b', 'c') s.append('/', signature='s') # 0'th argument is a string In the D-Bus Specification, dbus-daemon, and dbus-python for Python 2, the match rule matches s, but not o. In dbus-python for Python 3, because you removed the UTF8String type-check, it will match both. Switching away from UTF8String is great, but to keep the match rule semantics from the D-Bus Specification, you should replace the explicit dbus.UTF8String check with an explicit dbus.String check, rather than removing it. > >dbus_bindings is so deprecated that it should never have existed > > It does emit a DeprecationWarning currently for both versions, so raising an > exception (ImportError most likely) in Python 3 is an easy change. Let's just take the opportunity to remove it - people have had literally years to move away.
I've applied some of your less sweeping changes to master, to cut down the diffstat of the One Big Patch. This should hopefully indicate the sort of patches I would have preferred to see... Assigning this bug to you to mark it as "being worked on".
On Dec 13, 2011, at 12:08 PM, bugzilla-daemon@freedesktop.org wrote: >I've applied some of your less sweeping changes to master, to cut down the >diffstat of the One Big Patch. This should hopefully indicate the sort of >patches I would have preferred to see... > >Assigning this bug to you to mark it as "being worked on". Fantastic, thanks. I've managed to resolve my problems with github, so I will try to commit remaining changes in smaller chunks for easier review, and I'll continue to address your outstanding issues with Python 3 support.
On Dec 13, 2011, at 11:17 AM, bugzilla-daemon@freedesktop.org wrote: >> Would you rather see a patch that addresses all your comments, and which >> obsoletes the previous patches, or would you rather see one an incremental >> patch to apply on top of the ones you've already reviewed? > >Either works. To be honest, the easiest thing to review would have been a >series of "atomic" patches (e.g. "change the base types of all int subclasses >to be long under Python 3"), with the least controversial/most obvious changes >first - then I could start applying the stuff I liked already, and skip the >bits that still had problems (or depended on something problematic). Yes, I apologize for that. I know how difficult it is to review and apply massive diffs like the one I posted. The good news is that I see you've already cherry picked a bunch of changes that are useful on their own, or in preparation for Python 3 support. I've finally managed to get github working, so I've pushed a branch containing more of these types of atomic patches: https://github.com/warsaw/dbus-python3/tree/python3 So far, these are all changes you can accept without digesting the big Python 3 meal. They should make the eventual Python 3 patch much smaller. I'm happy to do the difficult work of pulling things out of the big diff into smaller diffs. Next up: PyString -> PyBytes, which will be large-ish, but completely compatible with Python 2. Then I'll probably do the reprs, followed by some of the PyInt -> PyLong changes. Then we'll see where we're at. :) >> Sounds right, I'll remove this from "user-visible changes". Since the class >> is unused in Python 2, should I #ifdef it out when compiled under Python 2? > >Ideally, yes. (re: #ifdef out _ByteBase in Python 2: will do). >> >Putting the "none of the above" case in the middle like you have here >... >> >guarantees that you'll have to rewrite the conditional if you add a third >> >thing. >> >> In this particular case, is there a reason not to accept either bytes or >> unicodes in both versions of Python? My rewrite based on your feedback >> does enable that, but if you'd rather I can add some #ifdefs to limit it to >> "native strings" (see below). > >This is for signatures, right? > >The fact that Python 2 doesn't allow unicode signatures is arguably a bug; if >you wanted to relax this, it's not a problem. > >Python 3 accepting bytes signatures would be a bit strange, but not really a >big deal, so if you wanted to allow this, I wouldn't mind. Cool. >> >This ordering is really weird. Any particular reason why they're not in >> >some sort of systematic ordering, like "sort by size and then by >> >signedness"? >> >> Mostly because I copied the ordering from the Python 2 PyInt and PyLong >> cases and then tried to combine them. I noticed the comment just before >> this stanza: >> >> /* Ordering is important: some of these are subclasses of each other. */ >> >> so I wanted to mess with that ordering as little as possible. > >Right, the ordering that comment refers to is (for instance) that the cases >for Int64 and UInt64 have to come before the case for plain 'long', and the >cases for ObjectPath and Signature have to come before the case for plain >'str' (whatever that means in the current version). > >None of the user-visible dbus-python types (Int64 etc.) are subclasses of each >other, so it's safe to re-order those. Cool, I'll clean up the ordering when I get to that part again. >> >This logic (to make a Byte from a single-byte unicode string) seems really >> >strange, and a bit inefficient. >> >> XXX COME BACK TO ME > >Still to be looked at Dang. I *knew* I'd forget to respond to that before I sent the message. I'll get back to this one in a subsequent follow up. >> The one reason why it silently swallows the argument is to aid in porting >> the existing uses of utf8_strings in the Python layer of the library. > >Yes... but I feel as though accepting an argument that doesn't do what it's >claimed to is rather dishonest. > >> It gets a little trickier at the Python layer. For example, there's a slot >> in SignalMatch for _utf8_strings, but it's essentially ignored. >> add_signal_receiver() also accepts but ignores it, as does call_async() and >> call_blocking() in connection.py. There may be other cases. The question >> is whether we want to track these down and add exceptions or deprecation >> warnings to nudge folks into getting rid of these (and in some cases, what >> to do about their use within the dbus-python package itself). Is it worth >> it to complicate the Python code, or break backward compatibility? > >I think grepping for utf8_strings should be enough to fix the internal uses? >Some cases will probably have to turn from > > foo(utf8_strings=utf8_strings) > >into > > kwargs = {} > if is_py3 and utf8_strings: > kwargs['utf8_strings'] = True > foo(**kwargs) > >but I think that's a necessary evil. > >Fundamentally, anyone who's relying on utf8_strings behaviour under Python 3 >has already lost, because they can't have it. Agreed; that's a good approach, and I'll adopt it as I port things over to the git branch. >> I looked into this in more detail, and AFAICT, connection.py and service.py >> really only wants the thread module for .allocate_lock(), so this one could >> easily be changed to use threading.Lock (or RLock?) objects. >> >> One thing I could not answer though is why _dbus.py imports thread. Maybe >> we can get rid of that and switch the other two to use threading? > >You know Python threads better than I do, tbh... I changed these to use threading.Lock() in the git branch. >> >Removing the UTF8String check is a semantic change: previously we insisted >> >that arguments matched with args_match were (in D-Bus terms) strings, >> >whereas now you'll accept anything string-like. >> >> Right, so the big question for you is whether that semantic and API change >> is acceptable. > >To be clear, here is the semantic change I don't like: consider this D-Bus >match rule: > > "arg0='/'" > >and these messages: > > o = dbus.lowlevel.SignalMessage('/', 'a.b', 'c') > o.append('/', signature='o') # 0'th argument is an object path > > s = dbus.lowlevel.SignalMessage('/', 'a.b', 'c') > s.append('/', signature='s') # 0'th argument is a string > >In the D-Bus Specification, dbus-daemon, and dbus-python for Python 2, the >match rule matches s, but not o. In dbus-python for Python 3, because you >removed the UTF8String type-check, it will match both. > >Switching away from UTF8String is great, but to keep the match rule semantics >from the D-Bus Specification, you should replace the explicit dbus.UTF8String >check with an explicit dbus.String check, rather than removing it. Thanks for explaining this. I'll make sure to adhere to the semantics you describe here when I get to that part in the Python 3 port again. I'm not sure if there are existing test cases for that; if not, I'll add one. Again, thanks. Hopefully with the github branch we have a better workflow for you to review and pull in the changes in more manageable chunks. If not, please let me know! Otherwise, I'll just follow up here when I get more stuff ready to go.
On Dec 12, 2011, at 04:58 PM, Barry Warsaw wrote: >>@@ +593,5 @@ >>> } >>> + y = *(unsigned char *)PyBytes_AS_STRING(obj); >>> + } >>> + else if (PyUnicode_Check(obj)) { >>> + PyObject *obj_as_bytes = PyUnicode_AsUTF8String(obj); >> >>This logic (to make a Byte from a single-byte unicode string) seems really >>strange, and a bit inefficient. >> >>If you're using UTF-8 (but why is UTF-8 special here?), then it'd be way more >>efficient to check that the length of the Unicode is 1 and the first (and >>only) character is < 128. (Otherwise, it'd encode to more than one UTF-8 >>byte.) >> >>But I think more sensible semantics would be to check that the length of the >>Unicode is 1, and use the numeric value of the first *codepoint* - so it's an >>error if it isn't in the latin-1 range, between U+0000 and U+00FF? >> >>(Optimization: even if Python is encoding its strings in UTF-16 like it does >>on Windows, it's enough to get the first-and-only Py_UNICODE - either a UCS-4 >>32-bit codepoint, or a 16-bit unit of UTF-16 which is either itself or half >>of a surrogate pair - and check that it's in the range 0 to 255. If it is, >>the right answer is that byte; if not, error.) Finally coming back to this issue. The (possible in)efficiency doesn't bother me at all. I don't think this is performance critical and while I haven't measured it, I'll bet the normal Python overhead will outweigh any conversions from unicode to bytes. The semantic question is more interesting though. Just what should it mean to append a byte signature with a unicode object? As an experiment, I commented out the PyUnicode_Check() stanza, and there were a few test failures in my current github branch. Looking at it more carefully though, I think it's better to disallow appending a bytes signature ('y') with anything other than a length-1 Python bytes object or an integer. Given that the semantics you outline above are questionable, "in the face of ambiguity, refuse the temptation to guess". So I'm all for disallowing unicode objects here. The downside is that users will have to change their code when porting from Python 2 to Python 3, because a native string (i.e. unadorned 8-bit string in Python 2) will not work in Python 3. It means prepending a b-prefix to specify byte strings. This doesn't seem bad to me, especially because we're already requiring a similar change for ByteArray instantiations. In the optimistic hope that you agree, I'll make this change (disallow unicodes with 'y' signatures), and update the user-visible changes section to reflect this. It's easy enough to back out of course. Cheers.
On Dec 13, 2011, at 11:17 AM, bugzilla-daemon@freedesktop.org wrote: >To be clear, here is the semantic change I don't like: consider this D-Bus >match rule: > > "arg0='/'" > >and these messages: > > o = dbus.lowlevel.SignalMessage('/', 'a.b', 'c') > o.append('/', signature='o') # 0'th argument is an object path > > s = dbus.lowlevel.SignalMessage('/', 'a.b', 'c') > s.append('/', signature='s') # 0'th argument is a string > >In the D-Bus Specification, dbus-daemon, and dbus-python for Python 2, the >match rule matches s, but not o. In dbus-python for Python 3, because you >removed the UTF8String type-check, it will match both. To be perfectly clear, is this the test that needs to pass? -----match.py----- from __future__ import print_function, unicode_literals from dbus.lowlevel import SignalMessage from dbus.connection import SignalMatch o = SignalMessage('/', 'a.b', 'c') s = SignalMessage('/', 'a.b', 'c') o.append('/', signature='o') s.append('/', signature='s') class Foo(object): pass def ignore(*args, **kws): pass m = SignalMatch(Foo(), None, '/', None, None, ignore, arg0='/') print(m, "matches 's' signature (should be True)?", m.maybe_handle_message(s)) print(m, "matches 'o' signature (should be False)?", m.maybe_handle_message(o)) -----match.py----- $ python match.py type='signal',path='/',arg0='/' matches 's' signature (should be True)? True type='signal',path='/',arg0='/' matches 'o' signature (should be False)? False If so, I've added essentially this test to test-standalone.py and fixed the code so that it passes in Python 2.x and 3.x. I think I've addressed all your previous comments so far. The github branch is up-to-date. https://github.com/warsaw/dbus-python3/tree/python3 Please re-review. The revisions should be small enough and incremental enough (with just the last few actually adding Python 3 support) to make it not as painful as last time. ;) Thanks!
Okay, my github branch now passes all tests for Python 2.6, 2.7, and 3.2 on Debian wheezy. I think it's ready to go. Cheers!
I've been on holiday for Christmas and now have some other work to catch up on, but I'll get to this when I can. Thanks!
(In reply to comment #70) > Please re-review. The revisions should be small enough and incremental enough > (with just the last few actually adding Python 3 support) to make it not as > painful as last time. ;) The "add Python 3 support" commit was still too big, but never mind. I've pushed a python3 branch based on yours to the official dbus-python repository; please review/test. I believe it's about ready for merging, and it passes tests on Python 2.6, 2.7, 3.2 under Debian sid. Changes made, relative to your branch: (In reply to comment #69) > The semantic question is more interesting though. Just what should it mean to > append a byte signature with a unicode object? I concluded that the answer is "nothing", and disallowed Byte('A') in the Python 3 world too. You now have to use Byte(ord('A')) or Byte(b'A') if that's what you want. (This is consistent with ByteArray.) I also moved the Python 2 API back to how it used to be: * things that used to return str (other than __str__/__repr__) are back to returning str, not unicode, via NATIVESTR_FROMSTR * things that used to return int (in particular, variant levels) still return int, not long, via a new NATIVEINT_FROMLONG so we won't break existing Python 2 code. In Python 3, those methods return long or unicode, of course. I also added an INTORLONG_CHECK macro so we don't keep repeating "is it a long (or in Python 2, an int)?" #ifdefs.
(http://cgit.freedesktop.org/dbus/dbus-python/log/?h=python3 is the new proposed branch.)
On Jan 11, 2012, at 03:19 PM, bugzilla-daemon@freedesktop.org wrote: >I've pushed a python3 branch based on yours to the official dbus-python >repository; please review/test. I believe it's about ready for merging, and it >passes tests on Python 2.6, 2.7, 3.2 under Debian sid. Reviewed and tested on Ubuntu precise against 2.6, 2.7, and 3.2. We'll have some experimental packages for Python 3.3 soon and I'll test them against that when they're available, but I don't expect changes to support 3.3 pre-alpha. (I can't test against a from-source install for dumb reasons.) >> The semantic question is more interesting though. Just what should it mean >> to append a byte signature with a unicode object? > >I concluded that the answer is "nothing", and disallowed Byte('A') in the >Python 3 world too. You now have to use Byte(ord('A')) or Byte(b'A') if that's >what you want. (This is consistent with ByteArray.) Agreed. >I also moved the Python 2 API back to how it used to be: > >* things that used to return str (other than __str__/__repr__) are back > to returning str, not unicode, via NATIVESTR_FROMSTR > >* things that used to return int (in particular, variant levels) still > return int, not long, via a new NATIVEINT_FROMLONG > >so we won't break existing Python 2 code. In Python 3, those methods return >long or unicode, of course. > >I also added an INTORLONG_CHECK macro so we don't keep repeating "is it a long >(or in Python 2, an int)?" #ifdefs. +1 to all of these. The branch looks great, and I appreciate all your help in getting this landed. From me, I think it's ready to merge! Cheers, -Barry
One other small change you'll need to apply: --- a/Makefile.am +++ b/Makefile.am @@ -20,6 +20,7 @@ nobase_python_PYTHON = \ dbus/bus.py \ dbus/connection.py \ + dbus/_compat.py \ dbus/_dbus.py \ dbus/_version.py \ dbus/decorators.py \
Finally, here is a diff that I've used to upload a new version to Ubuntu precise which includes the Python 3 packaging. Hopefully, you'll find it helpful for updating the Debian packaging; I'm interested in your feedback for this as well. I omitted the quilt patches and debian/changelong entry I used since they won't be relevant to Debian, or after you release the merged the `python3` branch. This also enables the test suite during the build, since that works now in precise. Apologies for pasting the diff here. Bugzilla rejected my attachment and I can't log into it atm. === modified file 'debian/control' --- debian/control 2011-07-25 12:51:22 +0000 +++ debian/control 2012-01-12 15:45:43 +0000 @@ -1,22 +1,32 @@ Source: dbus-python Section: devel Priority: optional -Maintainer: Utopia Maintenance Team <pkg-utopia-maintainers@lists.alioth.debian.org> +Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com> +XSBC-Original-Maintainer: Utopia Maintenance Team <pkg-utopia-maintainers@lists.alioth.debian.org> Uploaders: Sjoerd Simons <sjoerd@debian.org>, Sebastian Dröge <slomo@debian.org>, Simon McVittie <smcv@debian.org>, Loic Minier <lool@dooz.org> -Build-Depends: debhelper (>= 8), - xmlto, - python-all-dev (>= 2.6.6-3~), - python-all-dbg (>= 2.6.6-3~), +Build-Depends: autoconf, + automake, + autotools-dev, + debhelper (>= 8), libdbus-1-dev (>= 1.4), libdbus-glib-1-dev (>= 0.71), - autotools-dev -Build-Depends-Indep: python-docutils, - python-epydoc (>= 3.0~beta1) + libtool, + python-all-dbg (>= 2.6.6-3~), + python-all-dev (>= 2.6.6-3~), + python-gobject, + python-gobject-dbg, + python3-all-dbg, + python3-all-dev, + python3-gobject, + python3-gobject-dbg, + xmlto +Build-Depends-Indep: python-docutils, python-epydoc (>= 3.0~beta1) Standards-Version: 3.9.2 -X-Python-Version: >= 2.4 +X-Python-Version: >= 2.6 +X-Python3-Version: >= 3.2 XS-Dm-Upload-Allowed: yes Homepage: http://www.freedesktop.org/wiki/Software/DBusBindings#Python Vcs-Git: git://anonscm.debian.org/git/pkg-utopia/dbus-python.git @@ -25,16 +35,12 @@ Package: python-dbus Section: python Architecture: any -Depends: ${shlibs:Depends}, - ${misc:Depends}, - ${python:Depends} +Depends: ${misc:Depends}, ${python:Depends}, ${shlibs:Depends} Recommends: python-gobject | python-gtk (<< 2.10) -Suggests: python-dbus-doc, python-dbus-dbg +Suggests: python-dbus-dbg, python-dbus-doc Replaces: python2.4-dbus Conflicts: python2.4-dbus -Breaks: gnome-osd (<< 0.12.0), - gajim (<< 0.11.1), - python-qt4-dbus (<< 4.8.3-3) +Breaks: gajim (<< 0.11.1), gnome-osd (<< 0.12.0), python-qt4-dbus (<< 4.8.3-3) Provides: ${python:Provides} Description: simple interprocess messaging system (Python interface) D-Bus is a message bus, used for sending messages between applications. @@ -45,11 +51,23 @@ . See the dbus description for more information about D-Bus in general. +Package: python-dbus-common +Section: python +Priority: extra +Architecture: any +Depends: ${misc:Depends}, ${python:Depends}, ${shlibs:Depends} +Description: Common files for dbus-python. +Conflicts: python-dbus (<< ${source:Version}) + Package: python-dbus-dbg Section: debug Priority: extra Architecture: any -Depends: python-dbus (= ${binary:Version}), python-dbg, ${shlibs:Depends}, ${misc:Depends} +Depends: python-dbg, + python-dbus (= ${binary:Version}), + python-dbus-common, + ${misc:Depends}, + ${shlibs:Depends} Description: Debug build of the D-Bus Python interface This package provides a version of the python-dbus package built for debugging versions of Python. @@ -62,3 +80,34 @@ Description: Documentation for the D-Bus Python interface This package provides text and HTML documentation, and examples, for the python-dbus package. + +Package: python3-dbus +Section: python +Architecture: any +Depends: python-dbus-common, + ${misc:Depends}, + ${python:Depends}, + ${shlibs:Depends} +Recommends: python-gi +Suggests: python-dbus-dbg, python-dbus-doc +Provides: ${python:Provides} +Description: simple interprocess messaging system (Python interface) + D-Bus is a message bus, used for sending messages between applications. + Conceptually, it fits somewhere in between raw sockets and CORBA in + terms of complexity. + . + This package provides a Python interface to D-Bus. + . + See the dbus description for more information about D-Bus in general. + +Package: python3-dbus-dbg +Section: debug +Priority: extra +Architecture: any +Depends: python3-dbg, + python3-dbus (= ${binary:Version}), + ${misc:Depends}, + ${shlibs:Depends} +Description: Debug build of the D-Bus Python interface + This package provides a version of the python-dbus package built for + debugging versions of Python. \ No newline at end of file === added file 'debian/python-dbus-common.install' --- debian/python-dbus-common.install 1970-01-01 00:00:00 +0000 +++ debian/python-dbus-common.install 2012-01-12 14:04:25 +0000 @@ -0,0 +1,2 @@ +debian/tmp/usr/include/* +debian/tmp/usr/lib/pkgconfig/* === added file 'debian/python-dbus-dbg.install' --- debian/python-dbus-dbg.install 1970-01-01 00:00:00 +0000 +++ debian/python-dbus-dbg.install 2012-01-12 14:04:25 +0000 @@ -0,0 +1,1 @@ +debian/python-dbus-2*-dbg/usr/lib/python2.*/*-packages/* === added file 'debian/python-dbus.install' --- debian/python-dbus.install 1970-01-01 00:00:00 +0000 +++ debian/python-dbus.install 2012-01-12 14:04:25 +0000 @@ -0,0 +1,1 @@ +debian/tmp/usr/lib/python2.*/*-packages/* === added file 'debian/python3-dbus-dbg.install' --- debian/python3-dbus-dbg.install 1970-01-01 00:00:00 +0000 +++ debian/python3-dbus-dbg.install 2012-01-12 14:04:25 +0000 @@ -0,0 +1,1 @@ +debian/python-dbus-3*-dbg/usr/lib/python3/*-packages/* === added file 'debian/python3-dbus.install' --- debian/python3-dbus.install 1970-01-01 00:00:00 +0000 +++ debian/python3-dbus.install 2012-01-12 14:04:25 +0000 @@ -0,0 +1,1 @@ +debian/tmp/usr/lib/python3/*-packages/* === modified file 'debian/rules' --- debian/rules 2011-07-25 12:51:22 +0000 +++ debian/rules 2012-01-12 15:51:20 +0000 @@ -3,6 +3,8 @@ # Copyright © 2003 Daniel Stone <daniels@debian.org> # Copyright © 2006 Sjoerd Simons <sjoerd@debian.org> +DH_VERBOSE=1 + # These are used for cross-compiling and for saving the configure script # from having to guess our platform (since we know it already) DEB_HOST_GNU_TYPE ?= $(shell dpkg-architecture -qDEB_HOST_GNU_TYPE) @@ -13,6 +15,9 @@ PYVERS := $(shell pyversions --requested --version debian/control) PYDEFAULTVER := $(shell pyversions --default --version) +PY3VERS := $(shell py3versions --requested --version debian/control) +PY3DEFAULTVER := $(shell py3versions --default --version) + ifeq ($(DEB_BUILD_GNU_TYPE),$(DEB_HOST_GNU_TYPE)) CONFIGURE_FLAGS += --build=$(DEB_BUILD_GNU_TYPE) else @@ -47,6 +52,9 @@ build-%/build-stamp: build-%/configure-stamp dh_testdir PYTHON=/usr/bin/python$* $(MAKE) -C build-$* +ifeq (,$(findstring nocheck,$(DEB_BUILD_OPTIONS))) + $(MAKE) -C build-$* check +endif touch $@ build-doc/build-doc-stamp: build-doc/configure-stamp @@ -56,7 +64,12 @@ build: build-arch #build: build-indep -build-arch: $(PYVERS:%=build-%/build-stamp) $(PYVERS:%=build-%-dbg/build-stamp) + +build-arch: $(PYVERS:%=build-%/build-stamp) \ + $(PYVERS:%=build-%-dbg/build-stamp) \ + $(PY3VERS:%=build-%/build-stamp) \ + $(PY3VERS:%=build-%-dbg/build-stamp) + build-indep: build-doc/build-doc-stamp install-clean: @@ -64,40 +77,40 @@ dh_testroot dh_prep -install-%: build-%/build-stamp +install-%: dh_testdir dh_testroot - $(MAKE) -C build-$* install DESTDIR=$(CURDIR)/debian/python-dbus + $(MAKE) -C build-$* install DESTDIR=$(CURDIR)/debian/tmp # keep a copy of /usr/include/debian-python.h and # /usr/lib/pkgconfig/debian-python.pc to verify they match later - cp debian/python-dbus/usr/include/dbus-1.0/dbus/dbus-python.h debian/tmp-$*.h - cp debian/python-dbus/usr/lib/pkgconfig/dbus-python.pc debian/tmp-$*.pc + cp debian/tmp/usr/include/dbus-1.0/dbus/dbus-python.h debian/tmp-$*.h + cp debian/tmp/usr/lib/pkgconfig/dbus-python.pc debian/tmp-$*.pc -dbg-install-%: build-%-dbg/build-stamp +dbg-install-%: dh_testdir dh_testroot - $(MAKE) -C build-$*-dbg install DESTDIR=$(CURDIR)/debian/python-dbus-dbg - find debian/python-dbus-dbg ! -type d ! -name '*.so' -print0 | xargs -0 rm -f - find debian/python-dbus-dbg -depth -empty -exec rmdir {} \; + $(MAKE) -C build-$*-dbg install DESTDIR=$(CURDIR)/debian/python-dbus-$*-dbg + find debian/python-dbus-$*-dbg ! -type d ! -name '*.so' -print0 | xargs -0 rm -f + find debian/python-dbus-$*-dbg -depth -empty -exec rmdir {} \; + for i in $$(find debian/python-dbus-$*-dbg -name '*.so'); do \ + b=$$(basename $$i .so); \ + mv $$i $$(dirname $$i)/$${b}_d.so; \ + done -install-arch: build-arch install-clean $(PYVERS:%=install-%) $(PYVERS:%=dbg-install-%) - rm -f debian/python-dbus/usr/lib/python*/*-packages/*.la - rm -rf debian/python-dbus/usr/share/doc/deleteme - rm -f debian/python-dbus-dbg/usr/lib/python*/*-packages/*.la - rm -rf debian/python-dbus-dbg/usr/share/doc/deleteme +install-arch: build-arch install-clean \ + $(PYVERS:%=install-%) $(PYVERS:%=dbg-install-%) \ + $(PY3VERS:%=install-%) $(PY3VERS:%=dbg-install-%) + rm -f debian/tmp/usr/lib/python*/*-packages/*.la + rm -rf debian/tmp/usr/share/doc/deleteme # compare installed .pc and .h, asserting that the ones all versions # wanted are the same as what we ended up with - for v in $(PYVERS); do \ - diff --brief debian/python-dbus/usr/include/dbus-1.0/dbus/dbus-python.h \ + for v in $(PYVERS) $(PY3VERS); do \ + diff --brief debian/tmp/usr/include/dbus-1.0/dbus/dbus-python.h \ debian/tmp-$$v.h || exit 1; \ - diff --brief debian/python-dbus/usr/lib/pkgconfig/dbus-python.pc \ + diff --brief debian/tmp/usr/lib/pkgconfig/dbus-python.pc \ debian/tmp-$$v.pc || exit 1; \ done rm -f debian/tmp-*.pc debian/tmp-*.h - for i in $$(find debian/python-dbus-dbg -name '*.so'); do \ - b=$$(basename $$i .so); \ - mv $$i $$(dirname $$i)/$${b}_d.so; \ - done clean:: dh_testdir @@ -141,7 +154,8 @@ dh_link -s dh_compress -s -X.py -X.js dh_fixperms -s - dh_python2 -s + dh_python2 -s -v + dh_python3 -s -v dh_installdeb -s dh_shlibdeps -s dh_gencontrol -s @@ -149,4 +163,6 @@ dh_builddeb -s binary: binary-arch binary-indep + autoreconf + .PHONY: build clean binary-indep binary-arch binary install-arch install-clean
Created attachment 56390 [details] [review] Add dbus/_compat.py to nobase_python_PYTHON in Makefile.am. make install does not install dbus/_compat.py Pleaes find patch to fix this.
(In reply to comment #78) > Created attachment 56390 [details] [review] [review] > Add dbus/_compat.py to nobase_python_PYTHON in Makefile.am. > > make install does not install dbus/_compat.py Pleaes find patch to fix this. Already fixed in Comment #76 and in dbus-python 1.0.0.
(In reply to comment #79) > Already fixed in Comment #76 and in dbus-python 1.0.0. Sorry about that. I was using the python3 branch. I did not realise that it was allready merged into master. Kudos to Simon, Barry and John on the release. Who do we need to ping to get http://dbus.freedesktop.org/doc/dbus-python/NEWS.html updated?
(In reply to comment #80) > I was using the python3 branch. I did not realise that it > was allready merged into master. I've deleted the python3 branch (Barry's version, now merged) and the old py3k branch (John's version, not directly merged but used as inspiration by Barry) to avoid confusion. > Who do we need to ping to get > http://dbus.freedesktop.org/doc/dbus-python/NEWS.html updated? Me, apparently... it seems I forgot dbus-python releases work. Everything should be up to date now.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.