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 196120
Summary: | Review Request: gresistor | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Chitlesh GOORAH <chitlesh> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | jeff, panemade |
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-09-05 14:42:38 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: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Chitlesh GOORAH
2006-06-21 08:38:18 UTC
A few notes: 1) BuildArchitectures => BuildArch 2) The "mkdir" lines in the %install section are superfluous. 3) There are no packages GTK+, glade or pygtk in Fedora. The package builds, but will not install. Makes me wonder if package installation was tested. 4) Requires should probably be pygtk2 and pygtk2-libglade. Updated. Spec URL: http://beta.glwb.info/gresistor/gresistor.spec SRPM URL: http://beta.glwb.info/gresistor/gresistor-0.0.1-2.src.rpm I am not able to download Updated SRPM. Fixed and Updated. Spec URL: http://beta.glwb.info/gresistor/gresistor.spec SRPM URL: http://beta.glwb.info/gresistor/gresistor-0.0.1-2.src.rpm Not an official review as I'm not yet sponsored Mock build for i386 development gave /var/tmp/gresistor-0.0.1-2.fc6-root-mockbuild/usr/share/applications/gresistor.desktop: key "Categories" string list not semicolon-terminated, fixing You Need to add semicolon at end of Categories string list MUST Items: - MUST: rpmlint shows no error - MUST: dist tag is present - MUST: The package is named according to the Package Naming Guidelines. - MUST: The spec file name matching the base package gresistor, in the format gresistor.spec - MUST: This package meets the Packaging Guidelines. - MUST: The package is licensed with an open-source compatible license GPL. - MUST: The License field in the package gresistor.spec file did NOT match any file in tarball. - MUST: The sources used to build the package matches the upstream source, as provided in the spec URL. md5sum is correct. - MUST: This package did NOT owns all directories that it creates. - MUST: This package did not contain any duplicate files in the %files listing. - MUST: This package have a %clean section, which contains rm -rf $RPM_BUILD_ROOT. - MUST: This package used macros. - MUST: Document files are included like README. - MUST: This Package include a gresistor.desktop file, and that file is installed with desktop-file-install in the %install section successfully. - MUST: Package is calling ldconfig on postun post successfully. * Source URL is present. * BuildRoot is correct BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) * BuildRequires is correct * Able to see Desktop icon along with its icon You need to * Add license file in tarball * Make package belong to /usr/share/gresistor under %files as %dir %{python_sitelib}/gresistor The "gtk+" package that you require in the -2 version is GTK+ v1.2.10. The installation of this package works because the gtk+ packages in Fedora have an epoch set, so rpm/yum is really comparing the 1:1.2.0 epoch/version of the gtk+ package vs the 0:2.2 epoch/version of the requires line in the gresistor package. In any case, we do not want to pull in GTK+ 1.x when it's GTK+ 2.x that is really needed. Just drop the gtk+ dependency, the pygtk2-libglade dependency should be sufficient. Also, the licensing should be cleared up. The only evidence of a license that I could find was a "license = 'GPL'" setting in setup.py. Also, the SimpleGladeApp.py file seems to be licensed LGPL by a separate author. A minor nit, but "resistor" was misspelled in the description. Fixed and Updated. Spec URL: http://beta.glwb.info/gresistor/gresistor.spec SRPM URL: http://beta.glwb.info/gresistor/gresistor-0.0.1-3.src.rpm Upstream has been informed for the licence, but hasn't yet replied. I'm waiting upstream's release. Cannot access specfile: $ curl -I http://beta.glwb.info/gresistor/gresistor.spec curl: (7) couldn't connect to host http://chitlesh.funpic.de/rpm/gresistor.spec http://chitlesh.funpic.de/rpm/gresistor-0.0.1-3.src.rpm Files section still wrong: Package needs to own %dir %{_datadir}/%{name} *.pyo files should no longer be ghosted, see http://fedoraproject.org/wiki/Packaging/Python#head-e48d83dfeb5e671e2018d361d6e75d7e6c6e519c Updated http://chitlesh.funpic.de/rpm/gresistor.spec http://chitlesh.funpic.de/rpm/gresistor-0.0.1-5.src.rpm before, I forgot to increment from 3 to 4, that's why we are at 5 now :) Well, functionally good. I will review this. First review of gresistor: 1. From http://fedoraproject.org/wiki/Packaging/Guidelines : * Licensing - Well, it seems that this package is distributed under GPL (my recognition is that GPL is more strict than LGPL, so if the package includes the code of both GPL and LGPL, the license of the whole package is GPL, perhaps). However, it would be better that you ask for upstream to clarify the license (from the discussion above, it seems you have already did it). * Requires: - python <- required by pygtk2-libglade Also, this package requires python(abi) = 2.4. * Compiler flags - Well, usually CFLAGS="$RPM_OPT_FLAGS" is needed, however, how about for this package? This src package don't have any .c files and this is a NOARCH package, so CFLAGS should not be necessary. 2. From http://fedoraproject.org/wiki/PackagingDrafts/ScriptletSnippets : * Requires(post,postun) - Well, all of Requires(post,postun) seems unnecessary accoding to the URL above. (%post, %postun scriptlets are necessary), * GTK+ icon cache - No icons are installed under /usr/share/icons. Perhaps it is better that + create symlink under /usr/share/icons/hicolor/48x48/apps which points to /usr/share/gresistor/icon.png + fix (fedora-)gresistor.desktop + and call gtk-update-icon-cache 3. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines : = Nothing. 4. Other things I have noticed: = Nothing. (In reply to comment #13) > First review of gresistor: > > 1. From http://fedoraproject.org/wiki/Packaging/Guidelines : > > * Licensing > - Well, it seems that this package is distributed under > GPL (my recognition is that GPL is more strict than LGPL, > so if the package includes the code of both GPL and LGPL, > the license of the whole package is GPL, perhaps). > However, it would be better that you ask for upstream to > clarify the license (from the discussion above, it seems > you have already did it). Ok, I've opted for GPL > * Requires: > - python <- required by pygtk2-libglade > Also, this package requires python(abi) = 2.4. Fixed > * Compiler flags > - Well, usually CFLAGS="$RPM_OPT_FLAGS" is needed, however, > how about for this package? This src package don't have any .c > files and this is a NOARCH package, so CFLAGS should not be > necessary. Fixed > 2. From http://fedoraproject.org/wiki/PackagingDrafts/ScriptletSnippets : > > * Requires(post,postun) > - Well, all of Requires(post,postun) seems unnecessary accoding to > the URL above. (%post, %postun scriptlets are necessary), Fixed > * GTK+ icon cache > - No icons are installed under /usr/share/icons. Perhaps it is > better that > + create symlink under /usr/share/icons/hicolor/48x48/apps which > points to /usr/share/gresistor/icon.png > + fix (fedora-)gresistor.desktop > + and call gtk-update-icon-cache > Actually it already installs its own png at /usr/share/gresistor/icon.png, the use of GTK+ icon cache and touch --no-create %{_datadir}/icons/hicolor was to update gnome/kde menus just after the install of gresistor. Hence a kde/gnome restart is not required to update the icons in the gnome/kde menus. Updated http://chitlesh.funpic.de/rpm/gresistor.spec http://chitlesh.funpic.de/rpm/gresistor-0.0.1-6.src.rpm (In reply to comment #14) > Actually it already installs its own png at /usr/share/gresistor/icon.png, the > use of GTK+ icon cache and touch --no-create %{_datadir}/icons/hicolor was to > update gnome/kde menus just after the install of gresistor. > > Hence a kde/gnome restart is not required to update the icons in the gnome/kde > menus. Well, my opinition is that if this package uses a icon under /usr/share/gresistor, not under /usr/share/icons/hicolor, then touching /usr/share/icons/hicolor or recreating cache under /usr/share/icons/hicolor is not necessary. Updated http://chitlesh.funpic.de/rpm/gresistor.spec http://chitlesh.funpic.de/rpm/gresistor-0.0.1-7.src.rpm I've opted for: * GTK+ icon cache - No icons are installed under /usr/share/icons. Perhaps it is better that + create symlink under /usr/share/icons/hicolor/48x48/apps which points to /usr/share/gresistor/icon.png + fix (fedora-)gresistor.desktop + and call gtk-update-icon-cache Well, I think that the name of "icon.png" is somewhat troublesome. Perhaps using "gresistor.png" is better to aboid comflict from other packages. Updated: Spec URL: http://beta.glwb.info/gresistor/gresistor.spec SRPM URL: http://beta.glwb.info/gresistor/gresistor-0.0.1-8.src.rpm Okay. No problem. I am pleased to say that this package (gresistor) is APPROVED by me. Package Change Request ======================= Package Name: gresistor Short Description: GGnome resistor color code calculator Owners: chitlesh Branches: EL-5 EL-6 cvs done. The misformatted nature of the CVS request ("Branches" instead of "New Branches") confused the processing script a bit. Please ignore flag changes while I test out a fix. Setting this back to let me process the rest of the queue. ;) |