Bug 447456 - Review Request: gupnp-tools: a collection of dev tools utilising GUPnP and GTK+
Review Request: gupnp-tools: a collection of dev tools utilising GUPnP and GTK+
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity low
: ---
: ---
Assigned To: Denis Leroy
Fedora Extras Quality Assurance
:
Depends On: 446639 447457
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-19 19:32 EDT by Peter Robinson
Modified: 2008-10-27 06:45 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-27 06:45:16 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
denis: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Patch to fix g_thread_init crash issue (1.20 KB, patch)
2008-10-11 09:25 EDT, Denis Leroy
no flags Details | Diff

  None (edit)
Description Peter Robinson 2008-05-19 19:32:43 EDT
Spec URL: http://fedora.roving-it.com/rawhide/gupnp-tools.spec
SRPM URL: http://fedora.roving-it.com/rawhide/gupnp-tools-0.4-1.fc9.src.rpm

GUPnP is an object-oriented open source framework for creating UPnP
devices and control points, written in C using GObject and libsoup.
The GUPnP API is intended to be easy to use, efficient and flexible.

GUPnP-tools is a collection of developer tools utilising GUPnP and GTK+.
It features a universal control point application as well as a sample
DimmableLight v1.0 implementation.
Comment 1 Peter Robinson 2008-06-17 05:42:07 EDT
New version, new srpm. Spec is the same location

SRPM URL: http://fedora.roving-it.com/rawhide/gupnp-tools-0.6-1.fc9.src.rpm
Comment 3 Denis Leroy 2008-09-09 05:25:15 EDT
Some initial issues.

- desktop files should be installed with desktop-file-install. See

https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

- all 3 utilities core dumps on startup. Looking closer, a simply patch is required. Look for the calls to g_thread_init() in each of the utility source directories, and remove the call. g_thread_init() can only be called once, and it's called automatically by gtk_init(), so no need to call it explicitly.
Comment 4 Peter Robinson 2008-09-09 05:28:53 EDT
Hi Denis,

There is a new release of the package in testing to fix the g_thread_init issue. I just waiting for it to be released. I have also fixed up some issues in the spec file, and will also fixe the desktop file issue.

Thanks,
Peter
Comment 5 Peter Robinson 2008-09-09 09:29:13 EDT
Hi Denis,

I've uploaded a new srpm and spec file based on the test release I have. It should have fixed all the issues.

SRPM: http://pbrobinson.fedorapeople.org/gupnp-tools-0.6.1-1.fc9.src.rpm
Comment 6 Denis Leroy 2008-09-10 04:47:48 EDT
Hi Peter,

The 0.6.1 release is not available on the download site, which prevents me from verifying the tarball as part of the review process.

Also, the calls to g_thread_init() are still there and the tools still crash on startup. I would recommend to base the review on 0.6 for now and to write a patch to remove the 3 calls to g_thread_init().
Comment 7 Peter Robinson 2008-09-17 08:04:43 EDT
Hi Denis,

Removing the calls to g_thread_init on 0.6 had the apps crash for me with the following error on universal-cp and av-cp, network-light segfaulted on rawhide (all 3 gave the error on F-9 but ran fine without the patch):

# gupnp-universal-cp 

GLib-ERROR **: The thread system is not yet initialized.
aborting...
Aborted

So it looks like there is other issues.
Comment 8 Peter Robinson 2008-09-29 07:15:57 EDT
OK. There's a new 0.6.1 release out. I've tested this and it build and runs fine on the current rawhide.

I've done a scratch build in koji and tested the resulting binaries work on i386 rawhide
http://koji.fedoraproject.org/koji/taskinfo?taskID=849551

New SRPM.
http://pbrobinson.fedorapeople.org/gupnp-tools-0.6.1-1.fc9.src.rpm
Comment 9 Peter Robinson 2008-10-08 06:32:07 EDT
Hi Denis,

Any chance to retest this?
Comment 10 Denis Leroy 2008-10-11 09:25:17 EDT
Created attachment 320093 [details]
Patch to fix g_thread_init crash issue

Peter, do you have a F-10 system to test this ?

I've upgraded my system to F-10, and the GThread init crash still exist for all 3 utilities here. That's working on F-10 for you ? Maybe I'm seeing a different glib behavior because my system is dual-core, I don't know. I'm attaching the patch required for it to work on my system.
Comment 11 Peter Robinson 2008-10-11 09:33:17 EDT
Yes, I've tested it on F-10 on both i386 (netbook) and x86_64 (VM) and wasn't seeing any crash when running on F-10. The VM is setup with 2 cores, and the netbook while not real dual core is hyperthreaded. I'll test it later today.
Comment 12 Denis Leroy 2008-10-11 09:54:53 EDT
Matthias, I was wondering if you could shed some light on this. Peter and I are seeing a different runtime behavior of these gupnp tools. On my f10-system, the uv tools immediately abort because g_thread_init() is called twice. If I run the tool from the debugger, the gupnp tool explicitly calls gtk_init(), then g_thread_init() right below. On my system, gtk_init() also calls g_thread_init() (through atk_bridge_init(), bonobo_activation_init() and corba orb init()), hence the abort.

Any insights ?
Comment 13 Bastien Nocera 2008-10-11 10:36:06 EDT
From the API docs:
g_thread_init() might only be called once. On the second call it will abort with an error. If you want to make sure that the thread system is initialized, you can do this: 
if (!g_thread_supported ()) g_thread_init (NULL);

So either change your call to g_thread_init() to check whether threads are already supported, or run g_thread_init() before anything else (and if it still crashes, file a bug against whatever is not checking for g_thread_supported() before calling g_thread_init).
Comment 14 Denis Leroy 2008-10-11 12:10:31 EDT
> So either change your call to g_thread_init() to check whether
> threads are already supported.

Yes I read the API doc as well, and that's exactly what the patch I attached does.

What I don't understand is why gtk_init() calls g_thread_init() on my system and not on Peter's system...
Comment 15 Denis Leroy 2008-10-21 03:27:02 EDT
- License is GPLv2+
- rpmlint ok
- spec file is good
- source md5sum good
- packages build
- utilities function correctly , i386 and x86_64


2 minor things:

- according to guidelines, "--vendor=fedora" should be used for desktop-file-install

- the %files section could be written with just 3 lines:
  %{_datadir}/gupnp-tools
  %{_bindir}/gupnp*
  %{_datadir}/applications/*.desktop


If you're ok with the patch I submitted, consider the package APPROVED. I've tested the package on F-9 x86_64 and F10 i386.
Comment 16 Peter Robinson 2008-10-21 06:38:30 EDT
> - according to guidelines, "--vendor=fedora" should be used for
> desktop-file-install

Actually according to the packaging guidelines the Vendor should only be set if there isn't one already in use. They already use 'gupnp'
https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
 
> - the %files section could be written with just 3 lines:
>   %{_datadir}/gupnp-tools
>   %{_bindir}/gupnp*
>   %{_datadir}/applications/*.desktop

Yes, I'm aware of that but the reason I tend to do it the other way is that you don't get unexpected files that may be introduced on a new release. Its one of those 6 to one, hlaf a dozen to the other things.
Comment 17 Denis Leroy 2008-10-21 07:08:28 EDT
> Actually according to the packaging guidelines the Vendor should only be set if
> there isn't one already in use. They already use 'gupnp'

Sounds good.
Comment 18 Peter Robinson 2008-10-21 15:36:49 EDT
So is that approved?
Comment 19 Denis Leroy 2008-10-21 17:30:55 EDT
If you include the patch I provided, yes that's APPROVED.
Comment 20 Peter Robinson 2008-10-21 17:53:29 EDT
I'll include the patch but as specified above it was causing me problems and if others report the issues I saw I will revert it.
Comment 21 Peter Robinson 2008-10-21 17:56:08 EDT
New Package CVS Request
=======================
Package Name: gupnp-tools
Short Description: GUPnP-tools is a collection of dev tools utilising GUPnP and GTK+
Owners: pbrobinson
Branches: F-9
Comment 22 Denis Leroy 2008-10-21 18:11:08 EDT
> I'll include the patch but as specified above it was causing me problems

This one won't, as it checks whether the init call is needed or not, as suggested by the API documentation. I didn't pursue further why it's needed on my F-10 system and not on my other F-9 server, but it's related to ATK and Gnome assistive setups.
Comment 23 Kevin Fenzi 2008-10-23 16:05:46 EDT
cvs done.
Comment 24 Peter Robinson 2008-10-27 06:45:16 EDT
Built and in rawhide

Note You need to log in before you can comment on or make changes to this bug.