Merge lp://qastaging/~robru/bzr-dbus/glib into lp://qastaging/bzr-dbus

Proposed by Robert Bruce Park
Status: Merged
Approved by: Richard Wilbur
Approved revision: 55
Merged at revision: 54
Proposed branch: lp://qastaging/~robru/bzr-dbus/glib
Merge into: lp://qastaging/bzr-dbus
Diff against target: 204 lines (+26/-27)
2 files modified
activity.py (+13/-14)
tests/test_activity.py (+13/-13)
To merge this branch: bzr merge lp://qastaging/~robru/bzr-dbus/glib
Reviewer Review Type Date Requested Status
Richard Wilbur Needs Fixing
Review via email: mp+154595@code.qastaging.launchpad.net

Description of the change

Drop unused GObject imports.

To post a comment you must log in.
Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Thank you for removing the GObject dependencies.
Andrew Starr-Bochicchio (andrewsomething) wrote on 2013-01-27: #
> Robert,

> I was able to successfully run the test suite in a minimal Debian sid chroot
> using your branch with python-gi installed as well as with only python-gobject-2.

So it looks like it was already tested with good results.
+1

review: Approve
Revision history for this message
Robert Bruce Park (robru) wrote :

Can you merge this, Richard? Or do we need somebody else?

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Since Andrew Starr-Bochicchio (andrewsomething) already tested your branch back on 27 Jan 2013, I should have made my review with a vote of +2 and merged it!

review: Approve
Revision history for this message
Robert Bruce Park (robru) wrote :

Thanks for merging ;-)

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Robert,

I'm glad I could help with the merging--you're welcome. Thank you so
much for caring enough to write and update the code!

Sincerely,

Richard

Revision history for this message
Robert Bruce Park (robru) wrote :

On Mon, Mar 25, 2013 at 10:25:23PM -0000, Richard Wilbur wrote:
> I'm glad I could help with the merging--you're welcome. Thank you so
> much for caring enough to write and update the code!

Oh, Richard -- it doesn't look like it actually merged... did you use
the bzr tool? Just clicking 'merged' in launchpad doesn't do it.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

On Mon, Mar 25, 2013 at 4:31 PM, Robert Bruce Park
<email address hidden> wrote:
> Oh, Richard -- it doesn't look like it actually merged... did you use
> the bzr tool? Just clicking 'merged' in launchpad doesn't do it.

That might explain why it never filled in the "Merged at revision"
field. I guess I am ignorant of the actual method required to effect
the merge. I didn't use bzr because I figured your branch and the
mainline for bzr-dbus are both hosted in Launchpad.

I have just looked at the documentation regarding code hosting in
Launchpad and it referred me to bzr documentation regarding the
merges.

Launchpad says...
   To merge this branch: bzr merge lp:~robru/bzr-dbus/glib

I tried the following...

bzr launchpad-login richard-wilbur
bzr merge lp:~robru/bzr-dbus/glib

from which I receive the error message:
bzr: ERROR: Not a branch: "/home/rwilbur/".

This is not so much of a surprise since I have no branch at that path.
 I am not familiar with the syntax to merge from one branch hosted on
Launchpad to another. I tried...

bzr merge lp:~robru/bzr-dbus/glib -d lp:bzr-dbus --preview

and I get the error message:
bzr: ERROR: No WorkingTree exists for
"bzr+ssh://bazaar.launchpad.net/%2Bbranch/bzr-dbus/".

Which bzr tool do you refer to?

Revision history for this message
Robert Bruce Park (robru) wrote :

On Tue, Mar 26, 2013 at 04:10:30AM -0000, Richard Wilbur wrote:
> and I get the error message:
> bzr: ERROR: No WorkingTree exists for
> "bzr+ssh://bazaar.launchpad.net/%2Bbranch/bzr-dbus/".
>
> Which bzr tool do you refer to?

Hey Richard, sorry for the confusion. What you'll want to do would look
something more like this:

bzr branch lp:bzr-dbus
cd bzr-dbus
bzr merge lp:~robru/bzr-dbus/glib
bzr commit -m 'Drop obsolete GObject imports.'
bzr push :parent

Once that final 'push' happens, the merge will be complete. Thanks!

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

With...

bzr merge lp:~robru/bzr-dbus/glib -d lp:~bzr/bzr-dbus/trunk --preview

I get the following error message:

bzr: ERROR: No WorkingTree exists for "bzr+ssh://bazaar.launchpad.net/~bzr/bzr-dbus/trunk/".

So do I need to pull the trunk branch down, merge your branch into it, then push it up to trunk?

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Thanks for the tutorial, it worked like a charm. Looks like I had
just come to a similar conclusion but the help with the syntax is much
appreciated!

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Robert, the build is broken because of selftest failures. Looks like the change to the activity.py:331:io_add_watch call in the previous merge broke the syntax of the call. I installed python-testing and reproduced the error on my box. Fixed by removing the new second parameter of the call ("GLib.PRIORITY_HIGH, ").

Still have one failure of selftest bzrlib.plugins.dbus.tests.test_activity.TestActivity.test_server
in tests/test_activity.py:215:test_server where it is trying to invoke a server test that it received a signal.

This is a bit deeper and I haven't had the time to try and debug it yet.

How would you like to handle the fixes? Do you want to fix on this branch or shall I create a new branch?

review: Needs Fixing
Revision history for this message
Robert Bruce Park (robru) wrote :

Hmmmm, what system are you running? I looked into this a bit today and GLib.io_add_watch's source code indicates that calling it without the priority setting is deprecated. Are you using Quantal perhaps?

When I run 'nosetests' in the latest trunk, I get the following error:

http://paste.ubuntu.com/5679197/

And removing the priority setting, as you suggest, doesn't change that for me. Unfortunately I'm not very familiar with this codebase... Is there somebody else who knows more about this code that we could ask for help?

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Errors from bzr-dbus build after merge (Mar 25, links in references at
the bottom):
build 4400991: Quantal[1]
build 4400992: Precise[2]
build 4400982: Raring[3]

Notice the tracebacks in each build end with lines similar to:
  File "/build/buildd/bzr-dbus-0.1~bzr54~ppa83~raring1/activity.py",
line 331, in start
    GLib.io_add_watch(self.sock, GLib.PRIORITY_HIGH, GLib.IO_IN,
self.handle_network_packet)
TypeError: third argument not callable

When I run pydoc glib.io_add_watch, I get the following response:

glib.io_add_watch = io_add_watch(...)
    io_add_watch(fd, condition, callback, user_data=None) -> source id
      callable receives (fd, condition, user_data)
    Arranges for the fd to be monitored by the main loop for the
    specified condition. Condition is a combination of glib.IO_IN,
    glib.IO_OUT, glib.IO_PRI, gio.IO_ERR and gio.IO_HUB.

I'm running:
~/src/bzr-dbus$ bzr --version
Bazaar (bzr) 2.1.4
  Python interpreter: /usr/bin/python 2.6.5
  Python standard library: /usr/lib/python2.6
  Platform: Linux-2.6.32-45-generic-i686-with-Ubuntu-10.04-lucid
  bzrlib: /usr/lib/python2.6/dist-packages/bzrlib

Is the new priority setting parameter available as a named argument?
That looks like it could resolve the syntax issue.

When I look at the GLib docs[4], I see that g_io_add_watch() has the
syntax returned by pydoc above but g_io_add_watch_full() has the
'priority' parameter. Which documentation were you consulting?

The last person to merge changes into the tests on bzr-dbus trunk[5]
was Jelmer Vernooij[6] but that was back in July of 2011. I'm
interested in learning about dbus so I'm game to try but it would
certainly be faster to ask someone with experience in this area.

References:
[1] https://launchpad.net/~bzr/+archive/daily/+build/4400991/+files/buildlog_ubuntu-quantal-i386.bzr-dbus_0.1%7Ebzr54%7Eppa83%7Equantal1_FAILEDTOBUILD.txt.gz
[2] https://launchpad.net/~bzr/+archive/daily/+build/4400992/+files/buildlog_ubuntu-precise-i386.bzr-dbus_0.1%7Ebzr54%7Eppa83%7Eprecise1_FAILEDTOBUILD.txt.gz
[3] https://launchpad.net/~bzr/+archive/daily/+build/4400982/+files/buildlog_ubuntu-raring-i386.bzr-dbus_0.1%7Ebzr54%7Eppa83%7Eraring1_FAILEDTOBUILD.txt.gz
[4] https://developer.gnome.org/glib/stable/glib-IO-Channels.html#g-io-add-watch
[5] https://code.launchpad.net/~bzr/bzr-dbus/trunk
[6] https://launchpad.net/~jelmer

Revision history for this message
Robert Bruce Park (robru) wrote :

Ok, so... it looks like you are running Lucid, which is very near the end of it's support cycle. I would highly recommend upgrading, especially if you are doing development.

Also, when you run 'pydoc glib.io_add_watch' you're looking at the documentation for the old static pygtk bindings, which we are hopefully no longer using (although they get pulled in in the except block, they're not preferred). I was looking at the source code of GLib.io_add_watch directly, as it exists in Raring, at /usr/share/pyshared/gi/overrides/GLib.py. If you search for io_add_watch in that file (on raring), it shows a lot of glue logic to allow it to be called either with or without the priority setting, but it does trigger a deprecation warning if you don't use the priority value.

If you follow the build logs earlier on, they indicate that they want to install either python-gobject-2 OR python-gi, and are choosing the first one by default, which is wrong. You need python-gi package in order to get the correct GLib behavior that we merged. I'm not sure what branch has the packaging metadata (it's missing from trunk), but that should be updated to depend on python-gi now.

Revision history for this message
Robert Bruce Park (robru) wrote :

Richard, I've submitted a new merge proposal[0] with some code that may work around the issue you're seeing, but the better solution is to update the packaging to prefer python-gi package as I indicated in my previous message.

[0] https://code.launchpad.net/~robru/bzr-dbus/wrap-old-glib/+merge/157444

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Robert, I see your merge proposal. That's a cute trick, but I agree
with you that I would rather go to the root of the problem and fix the
packaging issue, if possible. Thank you for explaining what was going
on with the call syntax issue.

The only branch I was able to find with packaging information looks
like lp:~debian-bazaar/bzr-dbus/unstable and the ./debian/control [*]
file seems to specify the dependency on python-gobject-2 | python-gi

It seems to be suffering from an import issue since last November.

If I could fix the debian/control file, release the package into a
PPA, and update my bzr-dbus from the PPA, would that resolve the
dependency issue for the Ubuntu package?

Reference:
[*] http://bazaar.launchpad.net/~debian-bazaar/bzr-dbus/unstable/view/head:/debian/control

Revision history for this message
Robert Bruce Park (robru) wrote :

No, if you did it in a PPA, that would only fix it on your system (and on the systems of whoever else is subscribed to the PPA, if any). To fix it in ubuntu there's an official packaging branch somewhere, I'm just not sure exactly where at the moment.

I would guess that ubuntu:bzr-dbus would be the official packaging branch, but that branch seems to have the correct dependency on python-gi:

http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/raring/bzr-dbus/raring/view/head:/debian/control

So I can only guess that the problem arises from something you're specifically doing on your system? I recall you had mentioned using lucid, so that could be the source of the conflict.

If that's the case, then my other mp with the little API wrapper trick might actually be the best solution, as it provides a uniform API regardless of which version of glib is being used, allowing for maximum versatility.

Revision history for this message
Robert Bruce Park (robru) wrote :

You should try merging the mp and then seeing if that resolves the failures you mentioned seeing. You can do the 'bzr merge' step to test locally before doing 'bzr push' to officially accept the mp into trunk.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

I was looking for Ubuntu's official packaging branch and hadn't found
it. I understand that a PPA will only fix the problem for those who
update from it.

Thanks for checking into the Ubuntu Raring branch. Interesting that
it seems to have the correct dependency but the buildd system failed
on building bzr-dbus package for Raring with that same syntax error!
[1] It looks like the recipe for the daily builds references
different packaging on a different branch. I guess the next thing to
determine is how to update the branch the daily builds are made from.

Your API trick fixes the syntax problem but does it give a deprecation
warning with the new python-gi dependency? Does the 'priority'
argument convey a necessary semantic that we would leave out?

I notice that the first release to no longer depend on
python-gobject{-2} is raring.[2] So, if these changes aren't going to
update any other Ubuntu package, it seems better for me to update to
raring, myself, than dumb down the new code. If on the other hand the
bzr-dbus deprecation fixes will be used to update other Ubuntu
packages, we should stress the change in dependencies to the
maintainers.

References:
[1] https://launchpad.net/~bzr/+archive/daily/+build/4400982
[2] https://launchpad.net/ubuntu/+source/bzr-dbus

Revision history for this message
Robert Bruce Park (robru) wrote :

Richard, changes to lp:bzr-dbus wouldn't automatically affect anything pre-raring without going through a lengthy SRU process (in which the changes would be reviewed by multiple people who would prevent anything from breaking Quantal or Precise). On the other hand, I see now that there's a PPA that is set up to automatically build packages via a launchpad recipe[0], and it's configured to automatically build for precise, quantal, and raring.

Upon inspecting this recipe, it looks as though the packaging branch was trying to import from debian, but this has failed since last November[1]. I had a look at the debian branch that it's trying to import from, and it appears to have the correct dependency info. So this is the true cause of the failures.

I'm currently looking into getting this import running again[2], at which point this issue should just resolve itself.

[0] https://code.launchpad.net/~bzr/+recipe/bzr-dbus-daily
[1] https://code.launchpad.net/~debian-bazaar/bzr-dbus/unstable
[2] https://answers.launchpad.net/launchpad/+question/226513

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Robert, Thanks for checking into the buildd issue. I get messages
regarding the failures of these build recipes and I had followed it
far enough to see that the import branch started failing last November
and that the source branch was a redirect.

I look forward to working with the build after the import starts working again.

Thank you for keeping me in the loop and describing the process, I am
learning a lot and enjoying it.

Would it be significantly easier to maintain if we had the packaging
information in the source package or does it necessarily need to be on
a separate branch?

I understand a little about the SRU process so was skeptical this
would be a good candidate.

Revision history for this message
Robert Bruce Park (robru) wrote :

Merging the packaging information into the trunk branch is a possibility, and it's something that I've done with a number of other projects that I'm involved with, with good success. My only concern in this particular case is that it seems like the debian people are maintaining the "official" packaging, so if we do the inlining ourselves, then we cut ourselves off from getting updates from them in the future.

One thing I notice is that the "packaging" branch that we are importing from debian isn't really a packaging branch at all, it's a complete branch that happens to include the packaging. Seeing as the source of these errors is that we're mixing the trunk branch with an obsolete import of the packaging branch, it might make sense to just ignore the trunk branch for now and change the recipe to just only use the packaging branch by itself. This would ensure that nothing is ever out of sync again. What you should do is go to the recipe here:

https://code.launchpad.net/~bzr/+recipe/bzr-dbus-daily

And scroll down to the bottom where it says "Recipe contents". You should see a round yellow "edit" button there, and if you click on it it'll let you change the recipe. You should change it from this:

# bzr-builder format 0.3 deb-version {debupstream-base}bzr{revno}~ppa{revno:packaging}
lp:bzr-dbus
merge packaging lp:~debian-bazaar/bzr-dbus/unstable

To this:

# bzr-builder format 0.3 deb-version {debupstream-base}bzr{revno}
lp:~debian-bazaar/bzr-dbus/unstable

And then save the changes. That should actually stop the build failures even before the imports start working again, because we'll no longer be mixing the newer code with the older packaging (it'll all just be matching old code). Then once the imports start working again, everything will be nice and new and updated.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

My understanding of the daily builds is that they try to build the latest trunk using recent packaging information to catch problems the day after they hit the trunk.

To accomplish that goal we need some way to marry recent packaging to the tip of the trunk. I guess it would be useful to contact the maintainer of the Debian branch to coordinate efforts or find a way to use a recent Ubuntu packaging branch?

Revision history for this message
Robert Bruce Park (robru) wrote :

You're right, testing trunk is ideal, I was just considering workarounds for the current issue.

I've pursued this branch import issue a little bit with Canonical's IS team, and it looks like there's an issue importing any branches at all from debian, not just this one, so that's something they're working on internally.

I've pushed a copy of the branch manually, but unfortunately it's not possible to push it to the same location it normally imports to, so as a temporary workaround you'll still need to modify the recipe. Try going here:

https://code.launchpad.net/~bzr/+recipe/bzr-dbus-daily/+edit

And then change the recipe from this:

# bzr-builder format 0.3 deb-version {debupstream-base}bzr{revno}~ppa{revno:packaging}
lp:bzr-dbus
merge packaging lp:~debian-bazaar/bzr-dbus/unstable

to this:

# bzr-builder format 0.3 deb-version {debupstream-base}bzr{revno}~ppa{revno:packaging}
lp:bzr-dbus
merge packaging lp:~robru/+junk/bzr-dbus-unstable

And then that should solve the build failure you're currently seeing. Then we'd just have to keep an eye on the import issue and revert the recipe to it's original state once the imports are functioning again.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Thanks for all the footwork on this one! I am glad the Canonical IS
team is working on resolving the issue with debian branch imports.
Guess it was a bigger problem than just bzr-dbus.

Thank you also for providing a temporary, local solution. I followed
your directions, commented out the old 'merge packaging' line and
added the new one to the build recipe. Then I requested builds based
on the recipe. When I looked at the recipe, the lines I had commented
out with '#' had disappeared, so thank you for documenting the old
line in your message so I can restore it when debian imports start
working again.

The build succeeded on Raring but failed on Precise and Quantal, even
though it has the new dependency on python-gi and no dependency on
python-gobject-2!

Revision history for this message
Robert Bruce Park (robru) wrote :

Is it the same error message as before? That's quite strange. At this point I'd probably just disable the builds for precise and quantal...

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

It is the same error message as before: "activity.py", line 331, in start
    GLib.io_add_watch(self.sock, GLib.PRIORITY_HIGH, GLib.IO_IN,
self.handle_network_packet)
TypeError: third argument not callable

Occurs, distribution, python-gi version
  yes, "precise", 3.2.2-1~precise
  yes, "quantal", 3.4.0-1ubuntu0.1
  no, "raring", 3.8.0-2

So here's a patch that resolves the TypeError issue on "lucid" (still
fails test_server as it never sets started):

=== modified file 'activity.py'
--- old/activity.py 2013-03-21 05:57:40 +0000
+++ new/activity.py 2013-05-08 09:26:19 +0000
@@ -328,7 +328,12 @@
             self.port = self.sock.getsockname()[1]
         else:
             self.port = _port
- GLib.io_add_watch(self.sock, GLib.PRIORITY_HIGH, GLib.IO_IN,
self.handle_network_packet)
+ try:
+ # Priority arg first works in python-gi: 3.8.0-2 (raring)
+ GLib.io_add_watch(self.sock, GLib.PRIORITY_HIGH,
GLib.IO_IN, self.handle_network_packet)
+ except TypeError:
+ # Needed for python-gi: 3.2.2-1~precise, 3.4.0-1ubuntu0.1 (quantal)
+ GLib.io_add_watch(self.sock, GLib.IO_IN,
self.handle_network_packet)
         # listen for dbus events
         self.activity.listen_for_revisions(self.catch_dbus_revision)

Revision history for this message
Robert Bruce Park (robru) wrote :

Richard, your patch got mangled (wrapped) in the launchpad comments. Please resubmit it in the form of a merge proposal, like so:

bzr branch lp:bzr-dbus
cd bzr-dbus
[edit the file with your favorite editor]
bzr commit -m 'Fix GLib calls in Precise and Quantal.'
bzr push lp:~/bzr-dbus/fix-old-glib

Then visit here:

https://code.launchpad.net/~richard-wilbur/bzr-dbus/fix-old-glib/+register-merge

And fill out the form, and submit.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches