Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 842509
Summary: | Review Request: libdbusmenu - A helper library for libindicator | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jef Spaleta <jspaleta> |
Component: | Package Review | Assignee: | Michael S. <misc> |
Status: | CLOSED DUPLICATE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
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 okay I'll cycle back and do a mock build or two and get the deps in order. -jef Yup, confirmed misc's result, looks like missing BRs. Btw, could a tracker bug be added, so we can see the progression of unity packaging ? 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 (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. 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 ) (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? 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. 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. - 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. 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 ) 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. 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: (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 The 2 last issue seems to be fixed, so approved. 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 ) 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 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. 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. Any news here? @Jef, if you don't want to maintain this package anymore, I would take it over, trying to add gtk2 support. 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 *** This bug has been marked as a duplicate of bug 962029 *** |