Spec URL: http://jjames.fedorapeople.org/xgap/xgap.spec SRPM URL: http://jjames.fedorapeople.org/xgap/xgap-4.21-1.fc16.src.rpm Description: X Windows GUI for GAP.
Being a GUI application, this package needs a desktop file. It seems to me, the official website is http://www.gap-system.org/Packages/xgap.html and they also offer a bzip tarball: ftp://ftp.gap-system.org/pub/gap/gap4/tar.bz2/packages/xgap4r21.tar.bz2 Where did you find out about the license?
(In reply to comment #1) > Being a GUI application, this package needs a desktop file. Good point. I'll create one soon. > It seems to me, the official website is > > http://www.gap-system.org/Packages/xgap.html Everything under http://www.gap-system.org/Packages/ represents an attempt by the GAP developers to find and copy GAP packages to one place. However, their copies are often out of date. For that reason, I have been following the "WWW homepage" links on those pages to find the real upstream for each package. Note that the "WWW homepage" link on the XGap page is broken. It points to Max Neunhöffer's old web page. So I did a Google search for his current web page, found his XGap page, and listed that as the home page. I think that makes sense; it is the author's own web page for the package. > and they also offer a bzip tarball: > > ftp://ftp.gap-system.org/pub/gap/gap4/tar.bz2/packages/xgap4r21.tar.bz2 Hmmm, I'm not sure if I should use that, since that is a repacking of the upstream sources by a 3rd party. > Where did you find out about the license? In the manual; see doc/manual.pdf. The title page says, "We adopt the copyright regulations of GAP as detailed in the copyright notice in the GAP manaul." The GAP manual's copyright notice specifies GPL version 2 or any later version; see /usr/share/gap/doc/ref/manual.pdf, page 19. This appears to be common amongst GAP packagers. Many of the packages carry that identical phrase somewhere in the documentation.
I've added a desktop file. Same URLs as before: Spec URL: http://jjames.fedorapeople.org/xgap/xgap.spec SRPM URL: http://jjames.fedorapeople.org/xgap/xgap-4.21-1.fc16.src.rpm
The desktop file has a number of flaws. I therefore attached a modified copy. - Don't hardcode icons; I also wonder why this package has no icon but there is an icon and a copy of this desktop file in gap-core, which is no desktop application, as far as I can see. - Is there really a MIME type? If so, you need a scriptlet. - Would you really execute xgap handing over a list of URLs? (%U) - "Comment" is shown as context help for the application and should give the user a clue what that program does. You don't need a comment, but the original comment is not suitable. http://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html Please use the name macro consistently. You're using it on some occasions, but not on others, e. g. patch0. Please always change the release number and write to the changelog. The reviewer otherwise can easily miss out changes. I wonder if everything installed in /usr/share/gap/pkg/xgap is really necessary, for instance manual.dvi, manual.tex or Makefile. The same is true for other gap packages. You might also consider to install the necessary files with the doc macro and leave a link.
Created attachment 563928 [details] Improved desktop file for xgap
(In reply to comment #4) > The desktop file has a number of flaws. I therefore attached a modified copy. Thank you. > - Don't hardcode icons; I also wonder why this package has no icon but there is > an icon and a copy of this desktop file in gap-core, which is no desktop > application, as far as I can see. The icon comes with the GAP upstream distribution. The desktop file for GAP comes from Debian. It works, too, due to the "Terminal=true" in the GAP desktop file. Since this package is just an X Windows wrapper for GAP, it doesn't have its own icon, but borrows GAP's. However, it needs a different desktop file since it should have "Terminal=false" and a different executable name. > - Is there really a MIME type? If so, you need a scriptlet. The MIME type is defined by the gap-core package. The main GAP executable is already registered for that mime type, so this desktop file gives users a second choice of program to use to open such files. > - Would you really execute xgap handing over a list of URLs? (%U) Eek. No, that should be %f. Thanks for catching that. > - "Comment" is shown as context help for the application and should give the > user a clue what that program does. You don't need a comment, but the original > comment is not suitable. Thanks, I've adopted your suggestion for this. > Please use the name macro consistently. You're using it on some occasions, but > not on others, e. g. patch0. Fixed. > Please always change the release number and write to the changelog. The > reviewer otherwise can easily miss out changes. Sorry, I usually do that, but got into too much of a rush with this update and forgot until I had already uploaded the packages. It won't happen again. > I wonder if everything installed in /usr/share/gap/pkg/xgap is really > necessary, for instance manual.dvi, manual.tex or Makefile. The same is true > for other gap packages. You might also consider to install the necessary files > with the doc macro and leave a link. When I was working on the main GAP package, I tried dropping various files out of doc. No matter what I omitted, it made some part of the internal document viewer stop working. Even the source files are used, by an internal indexer. Even the READMEs are used, by an internal README viewer! I couldn't find anything that I could leave out without breaking something, so I gave up and left them all in. The same seems to be true for the GAP packages. I'll experiment with using %doc and a link. It makes me a little nervous, I'll admit, since my other experiments worked out so badly, but I'll try. Minus using %doc, all the other changes you requested are here: Spec URL: http://jjames.fedorapeople.org/xgap/xgap.spec SRPM URL: http://jjames.fedorapeople.org/xgap/xgap-4.21-2.fc16.src.rpm
Package Review ============== Key: - = N/A x = Pass ! = Fail ? = Not evaluated ==== C/C++ ==== [-]: MUST Header files in -devel subpackage, if present. [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 Package is not relocatable. ==== Generic ==== [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 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. [-]: 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. [x]: MUST Package contains a properly installed %{name}.desktop using desktop- file-install file if it is a GUI application. [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. [!]: MUST Permissions on files are set properly. [x]: MUST Package does not contain duplicates in %files. [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 [-]: 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. [x]: MUST Package consistently uses macros (instead of hard-coded directory names). [x]: MUST Package meets the Packaging Guidelines. [x]: MUST Package is named according to the Package Naming Guidelines. [x]: MUST Package does not generate any conflict. [x]: MUST Package obeys FHS, except libexecdir and /usr/target. [x]: 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 Requires correct, justified where necessary. [!]: MUST Rpmlint output is silent. rpmlint xgap-debuginfo-4.21-2.fc18.i686.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. rpmlint xgap-4.21-2.fc18.i686.rpm xgap.i686: E: non-standard-executable-perm /usr/bin/xgap.bin 0775L xgap.i686: W: no-manual-page-for-binary xgap xgap.i686: W: no-manual-page-for-binary xgap.bin 1 packages and 0 specfiles checked; 1 errors, 2 warnings. rpmlint xgap-4.21-2.fc18.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. [x]: MUST Sources used to build the package match the upstream source, as provided in the spec URL. /home/makerpm/785923/xgap4r21.zoo : MD5SUM this package : 160d5e8a4ddc872526ce57026cc92eb2 MD5SUM upstream package : 160d5e8a4ddc872526ce57026cc92eb2 [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 a SysV-style init script if in need of one. [x]: MUST File names are valid UTF-8. [x]: SHOULD Reviewer should test that the package builds in mock. [-]: 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. I suppose there is no active upstream anymore. [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. It starts, but I haven't tried further. [x]: SHOULD Package does not include license text files separate from upstream. [x]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: SHOULD Scriptlets must be sane, if used. [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. (Jan 1. 1970 though) [x]: SHOULD Spec use %global instead of %define. Issues: [!]: MUST Rpmlint output is silent. rpmlint xgap-debuginfo-4.21-2.fc18.i686.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. rpmlint xgap-4.21-2.fc18.i686.rpm xgap.i686: E: non-standard-executable-perm /usr/bin/xgap.bin 0775L xgap.i686: W: no-manual-page-for-binary xgap xgap.i686: W: no-manual-page-for-binary xgap.bin 1 packages and 0 specfiles checked; 1 errors, 2 warnings. rpmlint xgap-4.21-2.fc18.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Please set the permissions to 755. Besides that, the package is fine. Generated by fedora-review 0.1.2 External plugins:
Sorry to be silent so long. Real Life has been extremely busy recently. Here is the package with the permissions on the binary set to 0755. Spec URL: http://jjames.fedorapeople.org/xgap/xgap.spec SRPM URL: http://jjames.fedorapeople.org/xgap/xgap-4.21-2.fc16.src.rpm
Oops, let's try that again. Spec URL: http://jjames.fedorapeople.org/xgap/xgap.spec SRPM URL: http://jjames.fedorapeople.org/xgap/xgap-4.21-3.fc16.src.rpm
APPROVED
New Package SCM Request ======================= Package Name: xgap Short Description: GUI for GAP Owners: jjames Branches: f16 f17 InitialCC:
Git done (by process-git-requests).
xgap-4.21-3.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/xgap-4.21-3.fc17
xgap-4.21-3.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/xgap-4.21-3.fc16
xgap-4.21-3.fc17 has been pushed to the Fedora 17 testing repository.
xgap-4.21-3.fc16 has been pushed to the Fedora 16 stable repository.
xgap-4.21-3.fc17 has been pushed to the Fedora 17 stable repository.