Bug 806093
Summary: | Review Request: glade - User Interface Designer for GTK+ and GNOME | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Kalev Lember <kalevlember> |
Component: | Package Review | Assignee: | Rui Matos <tiagomatos> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | mclasen, notting, package-review, tiagomatos |
Target Milestone: | --- | Flags: | tiagomatos:
fedora-review+
gwync: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | glade-3.12.0-3.fc17 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2012-04-13 22:04:29 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
Kalev Lember
2012-03-22 21:06:25 UTC
Spec URL: http://kalev.fedorapeople.org/glade.spec SRPM URL: http://kalev.fedorapeople.org/glade-3.12.0-1.fc17.src.rpm * Wed Apr 04 2012 Kalev Lember <kalevlember> - 3.12.0-1 - Update to 3.12.0 Preliminary comments: %check desktop-file-validate $RPM_BUILD_ROOT%{_datadir}/applications/glade.desktop I think this should be part of the regular %install section - do we even run %check by default on builds ? %{_datadir}/gnome/help/glade/ %{_datadir}/omf/glade/ If you pass --with-gnome to %find_lang, it should pick these up (and properly decorate them with %lang) %files libs %doc COPYING* I think it would be nice to ship COPYING in the main package as well. (In reply to comment #2) > %check > desktop-file-validate $RPM_BUILD_ROOT%{_datadir}/applications/glade.desktop > > I think this should be part of the regular %install section - do we even run > %check by default on builds ? Yes, %check is run by default for all builds, right after %install. I don't think there's any practical difference whether to put this at the end of %install or in %check. https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage > %{_datadir}/gnome/help/glade/ > %{_datadir}/omf/glade/ > > If you pass --with-gnome to %find_lang, it should pick these up (and properly > decorate them with %lang) Fixed. > %files libs > %doc COPYING* > > I think it would be nice to ship COPYING in the main package as well. The licensing guidelines say that it's OK to put the license files in just one subpackage, provided that all other subpackages depend on the one that has license files. But sure, I guess it doesn't hurt to have multiple copies. Added COPYING* to the main package as well. https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Subpackage_Licensing Spec URL: http://kalev.fedorapeople.org/glade.spec SRPM URL: http://kalev.fedorapeople.org/glade-3.12.0-2.fc17.src.rpm * Fri Apr 06 2012 Kalev Lember <kalevlember> - 3.12.0-2 - Review fixes (#806093) - Use find_lang --with-gnome for including help files - Include license files also in the main package in addition to -libs * The .typelib should go in the -libs package * This License line is quite confusing: %package libs ... License: GPLv2+ and (GPLv2+ and LGPLv2+) and LGPLv2 Thanks, all very good points. Moved the typelib to -libs and updated the License tag along with the comment: > # - /usr/bin/glade is GPLv2+ > # - /usr/bin/glade-previewer is LGPLv2+ > # - libgladeui-2.so, libgladegtk.so, and libgladepython.so all combine > # GPLv2+ and LGPLv2+ code, so the resulting binaries are GPLv2+ > License: GPLv2+ and LGPLv2+ Spec URL: http://kalev.fedorapeople.org/glade.spec SRPM URL: http://kalev.fedorapeople.org/glade-3.12.0-3.fc17.src.rpm * Thu Apr 12 2012 Kalev Lember <kalevlember> - 3.12.0-3 - Update the spec file comments about licensing and simplify the License tag - Install the typelib in -libs subpackage Just one more thing which IMO isn't a blocker: %description devel The %{name}-devel package contains libraries and header files for developing applications that use %{name}. "applications that use glade" isn't strictly correct. Some (certainly very far from the majority) applications might use glade's widgets from the libgladeui-2 library but most won't have any dependency on any of these subpackages here. So, I think it should say: The %{name}-devel package contains libraries and header files for developing applications that use %{name}'s widgets library. Anyway, this looks APPROVED to me. Here's a more formal review and it actually caught a problem: + OK ! needs attention rpmlint output: glade.x86_64: W: obsolete-not-provided glade3 glade.x86_64: E: incorrect-fsf-address /usr/share/doc/glade-3.12.0/COPYING.LGPL glade.x86_64: E: incorrect-fsf-address /usr/share/doc/glade-3.12.0/COPYING.GPL glade.x86_64: W: no-manual-page-for-binary glade-previewer glade.x86_64: W: no-manual-page-for-binary glade glade-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/glade-3.12.0/gladeui/glade-accumulators.c glade-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/glade-3.12.0/src/glade-close-button.h glade-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/glade-3.12.0/src/glade-close-button.c glade-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/glade-3.12.0/gladeui/glade-clipboard.c glade-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/glade-3.12.0/plugins/gtk+/glade-fixed.c glade-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/glade-3.12.0/gladeui/glade-builtins.c glade-devel.x86_64: W: obsolete-not-provided glade3-libgladeui-devel glade-libs.x86_64: W: obsolete-not-provided glade3-libgladeui glade-libs.x86_64: E: incorrect-fsf-address /usr/share/doc/glade-libs-3.12.0/COPYING.LGPL glade-libs.x86_64: E: incorrect-fsf-address /usr/share/doc/glade-libs-3.12.0/COPYING.GPL 5 packages and 0 specfiles checked; 10 errors, 5 warnings. + Rpmlint warnings/errors are harmless and can be ignored + The package is named according to Fedora packaging guidelines + The spec file name matches the base package name. + The package meets the Packaging Guidelines + The package is licensed with a Fedora approved license and meets the Licensing Guidelines. + The license field in the spec file matches the actual license + The package contains the license file (COPYING) + Spec file is written in American English + Spec file is legible + Upstream sources match sources in the srpm. md5sum: bc743c2b94b674770b67cbc0c90fb3eb glade-3.12.0.tar.xz bc743c2b94b674770b67cbc0c90fb3eb glade-3.12.0.tar.xz.upstream + The package builds in koji n/a ExcludeArch bugs filed + BuildRequires look sane + The spec file handles locales properly + ldconfig in %post and %postun + Package does not bundle copies of system libraries + Package isn't relocatable + Package MUST own all the directories it creates + No duplicate files in %files + Permissions are properly set + Consistent use of macros + The package must contain code or permissible content n/a Large documentation files should go in -doc subpackage + Files marked %doc should not affect package + Header files should be in -devel n/a Static libraries should be in -static + Library files that end in .so must go in a -devel package + -devel must require the fully versioned base + Packages should not contain libtool .la files + Proper .desktop file handling ! Doesn't own files or directories already owned by other packages %doc %{_datadir}/gtk-doc/ This is taking ownership of both /usr/share/gtk-doc /usr/share/gtk-doc/html which doesn't look right. Instead the -devel package should require gtk-doc I believe. Hmmm, and now that I look at it, the build.log says: Build Reference Manual: no but here it is being installed. Sounds like an upstream bug, but gtk-doc should be added to the BuildRequires too and maybe explicitly enable gtk-doc in %configure. + Filenames are valid UTF-8 Multiple directory ownership is permitted to avoid depending on package which we wouldn't normally depend on. There's a section in the packaging guidelines that explains this, and specifically mentions the gtk-doc case: https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function > Hmmm, and now that I look at it, the build.log says: > > Build Reference Manual: no > > but here it is being installed. Sounds like an upstream bug, but gtk-doc should > be added to the BuildRequires too and maybe explicitly enable gtk-doc in > %configure. Upstream tarball ships with prebuilt documentation. --enable-gtk-doc is needed when we have to rebuild the docs, for example when building from git. But in this case we can use what's in the tarball. Oh ok, that sounds logical. So it's all good then. APPROVED again. Thanks for the review, Rui! New Package SCM Request ======================= Package Name: glade Short Description: User Interface Designer for GTK+ and GNOME Owners: kalev Branches: f17 InitialCC: Git done (by process-git-requests). Package imported and built; closing the ticket. |