Bug 842509

Summary: Review Request: libdbusmenu - A helper library for libindicator
Product: [Fedora] Fedora Reporter: Jef Spaleta <jspaleta>
Component: Package ReviewAssignee: Michael S. <misc>
Status: CLOSED DUPLICATE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: alekcejk, awilliam, echevemaster, mario.blaettermann, misc, notting, package-review, rdieter
Target Milestone: ---Flags: misc: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-05-11 09:02:21 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Jef Spaleta 2012-07-24 04:52:45 UTC
Spec URL: http://people.fedoraproject.org/~jspaleta/unity_deps/libdbusmenu.spec
SRPM URL: http://people.fedoraproject.org/~jspaleta/unity_deps/libdbusmenu-0.6.2-1.fc17.src.rpm
Description: A small little library created by pulling out some common code out
of indicator-applet. It passes a menu structure across DBus so that 
a program can create a menu simply without worrying about how it is 
displayed on the other side of the bus.

Fedora Account System Username: jspaleta

Comment 1 Michael S. 2012-07-24 21:00:53 UTC
Package fail to build ( on f17 and mock ) :

checking for gtkdoc-check... no
checking for gtkdoc-rebase... no
checking for gtkdoc-mkpdf... no
checking whether to build gtk-doc documentation... no
checking gnome-doc-utils >= 0.3.2... no
configure: error: gnome-doc-utils >= 0.3.2 not found

Comment 2 Jef Spaleta 2012-07-24 21:04:26 UTC
okay I'll cycle back and do a mock build or two and get the deps in order.

-jef

Comment 3 Adam Williamson 2012-07-25 19:41:45 UTC
Yup, confirmed misc's result, looks like missing BRs.

Comment 4 Michael S. 2012-07-25 20:14:23 UTC
Btw, could a tracker bug be added, so we can see the progression of unity packaging ?

Comment 5 Jef Spaleta 2012-07-25 22:12:38 UTC
sorry about that guys. added the one missing BR.

mock f17 run completes without error now for me
mock rawhide run completes without error now for me

ive pushed corrected spec and srpms to the same url with the one additional BR.
Please redownload.


-jef

Comment 6 Jef Spaleta 2012-07-26 03:05:48 UTC
(In reply to comment #4)

Let me be clear, I'm not currently working towards packaging "unity" so I don't see the point of a tracker bug.


My first goal is to get libunity packaged, as its libunity that applications are going to be building against to get access to the unity apis... regardless of whether unity the desktop shell built on top of compiz is running or not.

libdbusmenu is the only missing package that needs to get into the repository ahead of submitting libunity for review.  The other deps just need to be revved in rawhide which I now have access to do from Adam.

The first step towards packaging unity will be for someone to make the commitment to maintain compiz in Fedora. That person will not be me.

Comment 7 Michael S. 2012-07-26 06:04:58 UTC
The explicit requires on gtk3 is IMHO unwarranted, since rpm already detect it fine.

I also do not see why the main rpm requires vala, since that's a run time library.

The same goes for gobject-introspection, who is pulled twice by -devel ( one time explicitely, one time by the main rpm pulled by -devel ).

The Requires pkgconfig is already detected on newer rpm ( ie > EL 5 ), so can be dropped. ( like rm -rf $RPM_BUILD_ROOT in %install, btw ).


The multiple license of the tarbal requires more comments ( http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios )

And are the ldconfig call still needed ? ( real question, i think they were taken care by some automated script nowadays but maybe I am just dreaming )

Comment 8 Jef Spaleta 2012-07-26 08:42:52 UTC
(In reply to comment #7)
yes all those deps including the pkgconfig are redundant. Removed in latest version of the spec and srpm which are now uploaded back to the url locations.

Guidelines still have ldconfig as needed in post and postun.
https://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries

Some of the deps you mention were added due to my misreading of the current directory ownership policy. I've added the directories into the owned files list instead. You'll see the files section updated for all subpackages as a result of that correction.

I re-reviewed the licensing in the package. Only the tests are GPL'd. All the payload files in the binary rpm generated from C code that are explicitly licensed LGPLv2 OR LGPLv3 in the C file boiler plate headers. I've changed the License tag accordingly and added a note. 

Michael, does that address everything?

Comment 9 Michael S. 2012-07-26 09:40:36 UTC
Would it make sense to run the tests in %check ?

For the directory ownership policy, I was still living with the old unclear policy, but since you mentioned it, I reread it again and so it seems much more rational now, thanks.

I will start the formal review once I am back from work later today.

Comment 10 Michael S. 2012-07-26 09:44:43 UTC
In fact, some small questions :
- wouldn't it be easier to use %version in url ( so less work to update ) 

- since dbus-bench is not shipped in the rpm ( used for testing, and under GPL IIRC ), maybe there is no need to ship the README.dbusmenu-bench file in %doc ?

Also, i wonder why --disable-rpath is used and you still need to do chrpath -d, but maybe I just misunderstand what the option does, maybe something to bring to upstream.

Comment 11 Jef Spaleta 2012-07-26 16:52:54 UTC
- Upstream has some interesting patterns for source url directory tree across "series" I'd rather not assume anything about versioning and build the url by hand.

- dbus-bench is shipped in  the main package libexec  and its a python script and its explicitly LGPLv2 and LGPLv3 in the python script. I've confirmed that this shipped in U.'s packaging as well.  I should probably cycle back and split this off as a -tools subpackage and enable the building of the dumper and the testapp as well. Or just not include dbus-bench.


- yes the inability of disable-rpath to actually...disable rpath..is a bit of a mystery and I will make an effort to help upstream identify why its not working cleanly.  And the sed instructions that I have commented out, cause or sorts of havoc if applied in the build process.  As the upstream for this is very focused on a particular distribution channel, that does not have a similar rpath policy, it might take a bit of a negotation to get them to see it as an issue worth fixing. But I'll try to understand what's going wrong and submit a patch with a fix. Though I don't think getting that fixed should be a blocker for getting this into rawhide. The chrpath hack works for now, though not ideal.

Comment 12 Michael S. 2012-07-26 20:15:36 UTC
yeah, i just wondered.

Anyway, while doing the full review, i found out that %dir %{_datadir}/vala/ is likely unowned in the rpm, can you fix this ( I continue the review in the meantime, that's not a blocking issue )

Comment 13 Michael S. 2012-07-26 20:17:14 UTC
And there is no license installed if I install just the -doc subpackage ( ie, there is no deps on the main rpm, nor copy of the license in it ). And that's a blocker.

Comment 14 Michael S. 2012-07-26 20:22:50 UTC
Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST ldconfig called in %post and %postun if required.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Development (unversioned) .so files in -devel subpackage, if
     present.


==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: EXTRA Spec file according to URL is the same as in SRPM.
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[-]: MUST Macros in Summary, %description expandable at SRPM build time.
[-]: MUST Package contains desktop file if it is a GUI application.
[x]: MUST Development files must be in a -devel package
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Package complies to the Packaging Guidelines
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v3,)", "GPL (v2 or later)" For detailed output of licensecheck see
     file:
     /home/misc/checkout/git/FedoraReview/842509-libdbusmenu/licensecheck.txt
[!]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[-]: MUST If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[!]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Package is not relocatable.
[x]: MUST Package requires pkgconfig, if .pc files are present. (EPEL5)
     Note: Only applicable for EL-5
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains systemd file(s) if in need.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[-]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD The placement of pkgconfig(.pc) files are correct.
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX tarball generation or download is documented.
[x]: SHOULD SourceX / PatchY prefixed with %{name}.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Package must own all directories that it creates.
[!]: MUST License file installed when any subpackage combination is installed.

Rpmlint
-------
Checking: libdbusmenu-0.6.2-1.fc17.src.rpm
          libdbusmenu-devel-0.6.2-1.fc17.x86_64.rpm
          libdbusmenu-debuginfo-0.6.2-1.fc17.x86_64.rpm
          libdbusmenu-docs-0.6.2-1.fc17.x86_64.rpm
          libdbusmenu-0.6.2-1.fc17.x86_64.rpm
libdbusmenu.src: W: spelling-error Summary(en_US) libindicator -> lib indicator, lib-indicator, vindicator
libdbusmenu.src:18: W: mixed-use-of-spaces-and-tabs (spaces: line 18, tab: line 1)
libdbusmenu-devel.x86_64: W: no-documentation
libdbusmenu.x86_64: W: spelling-error Summary(en_US) libindicator -> lib indicator, lib-indicator, vindicator
5 packages and 0 specfiles checked; 0 errors, 4 warnings.


Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:
Requires
--------
libdbusmenu-devel-0.6.2-1.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    
    /usr/bin/pkg-config  
    gtk3-devel  
    libdbusmenu(x86-64) = 0.6.2-1.fc17
    libdbusmenu-glib.so.4()(64bit)  
    libdbusmenu-gtk3.so.4()(64bit)  
    pkgconfig(dbusmenu-glib-0.4)  
    pkgconfig(gdk-pixbuf-2.0)  
    pkgconfig(gtk+-3.0)  

libdbusmenu-debuginfo-0.6.2-1.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    

libdbusmenu-docs-0.6.2-1.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    

libdbusmenu-0.6.2-1.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    
    /sbin/ldconfig  
    /usr/bin/env  
    libatk-1.0.so.0()(64bit)  
    libc.so.6()(64bit)  
    libcairo-gobject.so.2()(64bit)  
    libcairo.so.2()(64bit)  
    libdbusmenu-glib.so.4()(64bit)  
    libgdk-3.so.0()(64bit)  
    libgdk_pixbuf-2.0.so.0()(64bit)  
    libgio-2.0.so.0()(64bit)  
    libglib-2.0.so.0()(64bit)  
    libgobject-2.0.so.0()(64bit)  
    libgtk-3.so.0()(64bit)  
    libpango-1.0.so.0()(64bit)  
    libpangocairo-1.0.so.0()(64bit)  
    libpthread.so.0()(64bit)  
    rtld(GNU_HASH)  

Provides
--------
libdbusmenu-devel-0.6.2-1.fc17.x86_64.rpm:
    
    libdbusmenu-devel = 0.6.2-1.fc17
    libdbusmenu-devel(x86-64) = 0.6.2-1.fc17
    pkgconfig(dbusmenu-glib-0.4) = 0.6.2
    pkgconfig(dbusmenu-gtk3-0.4) = 0.6.2

libdbusmenu-debuginfo-0.6.2-1.fc17.x86_64.rpm:
    
    libdbusmenu-debuginfo = 0.6.2-1.fc17
    libdbusmenu-debuginfo(x86-64) = 0.6.2-1.fc17

libdbusmenu-docs-0.6.2-1.fc17.x86_64.rpm:
    
    libdbusmenu-docs = 0.6.2-1.fc17
    libdbusmenu-docs(x86-64) = 0.6.2-1.fc17

libdbusmenu-0.6.2-1.fc17.x86_64.rpm:
    
    libdbusmenu = 0.6.2-1.fc17
    libdbusmenu(x86-64) = 0.6.2-1.fc17
    libdbusmenu-glib.so.4()(64bit)  
    libdbusmenu-gtk3.so.4()(64bit)  

MD5-sum check
-------------
https://launchpad.net/dbusmenu/0.6/0.6.2/+download/libdbusmenu-0.6.2.tar.gz :
  MD5SUM this package     : 80b782ac27e84d0ff531ee302a9aac03
  MD5SUM upstream package : 80b782ac27e84d0ff531ee302a9aac03


Generated by fedora-review 0.2.0 (a5c4ced) last change: 2012-07-22
Command line :./try-fedora-review -b 842509
External plugins:

Comment 15 Jef Spaleta 2012-07-29 20:50:43 UTC
(In reply to comment #14)
Hey Michael, 
can you repull and check to see if I get the final things fixed?

I removed the -bench utility and placed it in a commented out -tools package.
I'll turn on the tools psubackage later if there is a desire for those bits. 


-jef

Comment 16 Michael S. 2012-07-30 18:24:21 UTC
The 2 last issue seems to be fixed, so approved.

Comment 17 Michael S. 2012-09-22 23:39:36 UTC
Hi jef, since you seems to be back from your cold cold hideout, can you make the request for inclusion ( I approved the package, but since you left for a mission, I guess you may have forgot about this bug )

Comment 18 Mario Blättermann 2012-12-08 20:49:13 UTC
Would be nice to see this included in Fedora soon. Currently I've review requests for appmenu-qt (bug #882508) and plasma-widget-menubar (bug #882512). The Qt part works fine in the Plasma applet. Additionally, I'm planning to package appmenu-gtk and firefox-globalmenu [1] which adds appmenu support to Firefox and Thunderbird.

[1] https://launchpad.net/globalmenu-extension

Comment 19 Mario Blättermann 2012-12-16 21:31:09 UTC
BTW, this package builds for gtk3 only. Well, there are configure switches for both (--with-gtk2 and --with-gtk3) but they cannot be used simultaneously. This way we get menu bar support for gtk3 apps only. Hopefully there's a painless way to implement both parts in one srpm. This can be done later, so that I leave the "fedora-review" flag untouched.

However, once it is in Fedora, we have to crack another problem: To display the menu bar, patching the gtk sources is needed. At least the Ubuntu folks do so, and I haven't found another solution from other distributions. Would there be a way to get the gtk3 sources patched in Fedora at least, or even upstream? For the latter case, I don't expect that they implement the menu bar stuff in the (actually deprecated) gtk2 sources.

Comment 20 Adam Williamson 2012-12-17 22:26:15 UTC
Mario: there isn't really a _painless_ way to build for both gtk2 and gtk3, but it's possible. Basically you have to create two build directories and build / install it twice. libindicator itself does this, so you can just look at the libindicator spec for tips on how to get it done.

Comment 21 Mario Blättermann 2013-01-04 21:06:35 UTC
Any news here? @Jef, if you don't want to maintain this package anymore, I would take it over, trying to add gtk2 support.

Comment 22 Eduardo Echeverria 2013-05-11 03:37:55 UTC
I talked with @jef on IRC, a few months ago, i need this package as dependency of libappindicator. Here the new review https://bugzilla.redhat.com/show_bug.cgi?id=962029

Comment 23 Mario Blättermann 2013-05-11 09:02:21 UTC

*** This bug has been marked as a duplicate of bug 962029 ***