Bug 26420 - Patch to make D-Bus Python compile under Python 3
Patch to make D-Bus Python compile under Python 3
Status: RESOLVED FIXED
Product: dbus
Classification: Unclassified
Component: python
unspecified
Other All
: medium normal
Assigned To: Barry Warsaw
John (J5) Palmieri
https://github.com/warsaw/dbus-python...
review?
: patch
: 37982 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-03 15:07 UTC by John (J5) Palmieri
Modified: 2012-02-01 02:15 UTC (History)
9 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch to make dbus-python compile and work in python 3 (66.92 KB, patch)
2010-02-03 15:08 UTC, John (J5) Palmieri
Details | Splinter Review
Fix Raise and except to match python 3.x semantics (6.05 KB, patch)
2010-10-26 20:15 UTC, sjohnson
Details | Splinter Review
A new Python 3 port of dbus-python (208.02 KB, patch)
2011-12-05 15:51 UTC, Barry Warsaw
Details | Splinter Review
Additional patch to clean some things up (2.94 KB, patch)
2011-12-10 08:41 UTC, Barry Warsaw
Details | Splinter Review
Add dbus/_compat.py to nobase_python_PYTHON in Makefile.am. (1.11 KB, patch)
2012-01-31 06:43 UTC, Gary van der Merwe
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description John (J5) Palmieri 2010-02-03 15:07:47 UTC
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.
Comment 1 John (J5) Palmieri 2010-02-03 15:08:43 UTC
Created attachment 33044 [details] [review]
Patch to make dbus-python compile and work in python 3
Comment 2 John (J5) Palmieri 2010-02-03 15:11:30 UTC
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.
Comment 3 Simon McVittie 2010-02-04 03:51:32 UTC
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.
Comment 4 John (J5) Palmieri 2010-02-04 07:47:38 UTC
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.
Comment 5 John (J5) Palmieri 2010-02-04 08:55:20 UTC
Hmmm, I can't create the branch.  'Read-only file system' even though I cloned through ssh.  Am I still in the commit group?
Comment 6 Colin Walters 2010-02-04 09:00:09 UTC
Yep:

walters@annarchy:~$ groups johnp
freedesktop dbus
walters@annarchy:~$

Are you sure you have the right checkout url?
Comment 7 John (J5) Palmieri 2010-02-04 09:05:55 UTC
git clone git+ssh://johnp@git.freedesktop.org/git/dbus/dbus-python.git
Comment 8 John (J5) Palmieri 2010-02-04 09:21:51 UTC
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
Comment 9 John (J5) Palmieri 2010-02-04 09:36:36 UTC
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
Comment 10 Dave Malcolm 2010-02-04 11:56:25 UTC
(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.
Comment 11 John (J5) Palmieri 2010-02-07 14:41:21 UTC
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.
Comment 12 John (J5) Palmieri 2010-02-07 15:03:00 UTC
fixed the .ver file issue with this in Makefile.am -export-symbols-regex (PyInit_|init)_dbus_bindings
Comment 13 John (J5) Palmieri 2010-02-07 18:43:45 UTC
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. 
Comment 14 Simon McVittie 2010-02-08 05:47:10 UTC
> (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.
Comment 15 John (J5) Palmieri 2010-02-08 08:29:16 UTC
(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.
Comment 16 Simon McVittie 2010-02-08 09:02:52 UTC
(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.
Comment 17 John (J5) Palmieri 2010-02-08 10:05:38 UTC
(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.
Comment 18 John (J5) Palmieri 2010-02-08 10:24:01 UTC
(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.
Comment 19 John (J5) Palmieri 2010-02-15 20:39:20 UTC
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).
Comment 20 John (J5) Palmieri 2010-02-17 11:25:33 UTC
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?
Comment 21 Andrew Zaborowski 2010-04-19 10:50:24 UTC
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.
Comment 22 John (J5) Palmieri 2010-04-21 07:31:07 UTC
(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.
Comment 23 Simon McVittie 2010-04-21 07:41:06 UTC
(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).
Comment 24 BLACK 2010-10-24 03:56:55 UTC
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 :)
Comment 25 John (J5) Palmieri 2010-10-25 09:54:22 UTC
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]
    ...
Comment 26 BLACK 2010-10-26 05:38:27 UTC
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 :)
Comment 27 sjohnson 2010-10-26 20:15:21 UTC
Created attachment 39800 [details] [review]
Fix Raise and except to match python 3.x semantics
Comment 28 sjohnson 2010-10-26 20:16:22 UTC
Comment on attachment 39800 [details] [review]
Fix Raise and except to match python 3.x semantics

fixes problems recently complained about with compiling this
Comment 29 sjohnson 2010-10-26 20:18:40 UTC
(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.
Comment 30 John (J5) Palmieri 2010-10-26 20:32:39 UTC
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
Comment 31 sjohnson 2010-10-27 21:35:05 UTC
(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.
Comment 32 BLACK 2010-10-28 01:08:52 UTC
(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
Comment 33 sjohnson 2010-10-28 01:29:49 UTC
(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.
Comment 34 John (J5) Palmieri 2010-10-28 08:40:54 UTC
(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 35 John (J5) Palmieri 2010-10-28 08:42:00 UTC
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
Comment 36 Simon McVittie 2010-10-28 08:50:25 UTC
(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.)
Comment 37 John (J5) Palmieri 2010-10-28 09:04:35 UTC
(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?
Comment 38 sjohnson 2010-10-28 16:00:33 UTC
(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.
Comment 39 John (J5) Palmieri 2010-10-29 00:36:32 UTC
(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.
Comment 40 Denis Falqueto 2011-04-27 22:46:43 UTC
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).
Comment 41 John (J5) Palmieri 2011-04-28 09:08:29 UTC
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.
Comment 42 Simon McVittie 2011-04-28 09:19:12 UTC
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.
Comment 43 Denis Falqueto 2011-04-28 13:35:06 UTC
(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?
Comment 44 John (J5) Palmieri 2011-04-28 15:07:12 UTC
(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.
Comment 45 toddrme2178 2011-04-28 21:03:37 UTC
Making dbus depend on Gnome doesn't seem like a good approach for KDE users.
Comment 46 Simon McVittie 2011-04-29 02:45:43 UTC
(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.
Comment 47 Thiago Macieira 2011-04-29 03:56:23 UTC
> (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.
Comment 48 John (J5) Palmieri 2011-04-29 09:07:24 UTC
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.
Comment 49 Denis Falqueto 2011-04-29 09:27:46 UTC
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.
Comment 50 Simon McVittie 2011-06-06 06:43:52 UTC
*** Bug 37982 has been marked as a duplicate of this bug. ***
Comment 51 Barry Warsaw 2011-12-01 07:34:37 UTC
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.
Comment 52 Simon McVittie 2011-12-01 07:56:43 UTC
(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?)
Comment 53 Simon McVittie 2011-12-01 07:59:41 UTC
(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.
Comment 54 Barry Warsaw 2011-12-01 08:36:45 UTC
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. :)
Comment 55 Barry Warsaw 2011-12-01 09:00:35 UTC
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.
Comment 56 Simon McVittie 2011-12-01 09:49:20 UTC
> >(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.
Comment 57 Barry Warsaw 2011-12-05 15:51:14 UTC
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!
Comment 58 Barry Warsaw 2011-12-10 08:41:21 UTC
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 59 Simon McVittie 2011-12-12 04:28:25 UTC
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 60 Simon McVittie 2011-12-12 04:37:16 UTC
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.
Comment 61 Simon McVittie 2011-12-12 04:41:21 UTC
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
Comment 62 Simon McVittie 2011-12-12 04:42:41 UTC
(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.)
Comment 63 Barry Warsaw 2011-12-12 13:58:48 UTC
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
Comment 64 Barry Warsaw 2011-12-12 14:17:28 UTC
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?
Comment 65 Simon McVittie 2011-12-13 03:17:19 UTC
(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.
Comment 66 Simon McVittie 2011-12-13 04:08:38 UTC
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".
Comment 67 Barry Warsaw 2011-12-13 07:38:15 UTC
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.
Comment 68 Barry Warsaw 2011-12-13 15:08:13 UTC
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.
Comment 69 Barry Warsaw 2011-12-15 13:49:41 UTC
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.
Comment 70 Barry Warsaw 2011-12-15 16:45:09 UTC
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!
Comment 71 Barry Warsaw 2011-12-17 09:12:14 UTC
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!
Comment 72 Simon McVittie 2012-01-04 06:25:05 UTC
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!
Comment 73 Simon McVittie 2012-01-11 07:19:20 UTC
(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.
Comment 74 Simon McVittie 2012-01-11 07:20:17 UTC
(http://cgit.freedesktop.org/dbus/dbus-python/log/?h=python3 is the new proposed branch.)
Comment 75 Barry Warsaw 2012-01-12 02:46:12 UTC
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
Comment 76 Barry Warsaw 2012-01-12 06:13:29 UTC
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 \
Comment 77 Barry Warsaw 2012-01-13 01:19:42 UTC
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
Comment 78 Gary van der Merwe 2012-01-31 06:43:21 UTC
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.
Comment 79 Simon McVittie 2012-01-31 08:13:36 UTC
(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.
Comment 80 Gary van der Merwe 2012-01-31 23:40:39 UTC
(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?
Comment 81 Simon McVittie 2012-02-01 02:15:47 UTC
(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.