Bug 915144 (rasmol)

Summary: Review Request: rasmol - Molecular Graphics Visualization Tool
Product: [Fedora] Fedora Reporter: Dmitrij S. Kryzhevich <kryzhev>
Component: Package ReviewAssignee: Björn 'besser82' Esser <besser82>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: besser82, carl, i, notting, richardfearn
Target Milestone: ---Flags: besser82: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: rasmol-2.7.5-10.fc20 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-09-24 00:23:39 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 Dmitrij S. Kryzhevich 2013-02-25 04:22:52 UTC 
Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-1.fc18.src.rpm
Description:
RasMol is a molecular graphics program intended for the visualization of
proteins, nucleic acids and small molecules. The program is aimed at
display, teaching and generation of publication quality images.
Fedora Account System Username: krege

Known issues:
1) itrasmol2721.hlp is not in utf. As I understand, it should not and so it is not a prolem.
2) incorrect fsf address in gplv2 License file. In general: old text of GPLv2 License.
I do not think it is possible to contact an author and ask him to provide updated version. Is it allowd to change the text of files that contain license terms? It is, yes, GPLv2 and updated file is the same GPLv2 but still there is a question.
3) some man pages minor warnings.
Comment 1 Dmitrij S. Kryzhevich 2013-02-25 04:24:04 UTC 
*** Bug 541462 has been marked as a duplicate of this bug. ***
Comment 2 Dmitrij S. Kryzhevich 2013-02-25 04:32:42 UTC 
Issue 2:
https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address
I do not allowed to change this file, need to provide this "as is".
Comment 3 Dmitrij S. Kryzhevich 2013-03-03 03:23:12 UTC 
Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-2.fc18.src.rpm

An error in script: missed "-e" option to the xterm.
Comment 4 Dmitrij S. Kryzhevich 2013-03-04 09:46:22 UTC 
Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-3.fc18.src.rpm

Move some examples and docs to -doc subpackage.

Warning. Source codes are not available on official site at the moment with 403 error. I wrote a letter to the webmaster, waiting for the reply.
Comment 5 Dmitrij S. Kryzhevich 2013-03-05 04:05:16 UTC 
Warning droppped.
Sources available now.
Comment 6 Antonio T. (sagitter) 2013-03-11 14:37:40 UTC 
Hi Dmitrij.

I'm not a official Fedora packager so let me try to review your package. My review could contain some (my) evaluation errors, please you consider all that as a test for myself. :)

- gzip can be omitted
  https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Exceptions_2

- Another license (LGPLv2+) has been detected for 'eggfileformatchooser.h' file. You should consider to add a multiple licensing:
  https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

- Patches link to upstream bugs/comments/lists or are otherwise justified. These info are missing, add them if possible.

- Package uses nothing in %doc for runtime.
  -doc sub-package contains '%{_datadir}/%{name}/data' directory with all *.pdb* files; .pdb files are Protein Data Bank data files used from application. 
Why they are excluded from main package ? If they were not, -doc sub-package could be packaged as 'noarch' and could contain only document files.

- Fully versioned dependency in subpackages, if present.
  I don't know if Epochs is strictly necessary for rasmol; if no, 
  'Requires: %{name}%{?_isa} = %{version}-%{release}' is better.

- Package consistently uses macro is (instead of hard-coded directory names).
  %%{name} macro can be used more frequently in .spec file.

- rasmol-doc.x86_64: W: file-not-utf8 /usr/share/doc/rasmol-doc-2.7.5/doc/itrasmol2721.hlp
This warning seems can be fixed by using something like that:

iconv --from=ISO-8859-1 --to=UTF-8 doc/itrasmol2721.hlp > doc/itrasmol2721_new.hlp 
mv doc/itrasmol2721_new.hlp doc/itrasmol2721.hlp

http://fedoraproject.org/wiki/Packaging_tricks#Convert_encoding_to_UTF-8

Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that are
  listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: gzip
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
- update-desktop-database is invoked when required
  Note: desktop file(s) in rasmol, rasmol-gtk
  See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[?]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[-]: Package requires other packages for directories it uses.
[!]: Patches link to upstream bugs/comments/lists or are otherwise justified
[x]: Package is not known to require ExcludeArch.
[!]: Fully versioned dependency in subpackages, if present.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rasmol-gtk
     , rasmol-doc
[x]: Package complies to the Packaging Guidelines
[?]: 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.
[?]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "LGPL (v2 or later) (with incorrect FSF address)", "Unknown or
     generated". 2 files have unknown license. Detailed output of licensecheck
     in /home/sagitter/rpmbuild/SRPMS/915144-rasmol/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[!]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[!]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: gtk-update-icon-cache is invoked when required
     Note: icons in rasmol
[x]: Useful -debuginfo package or justification otherwise.
[x]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 481280 bytes in 20 files.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[!]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[!]: Package does not include license text files separate from upstream.
[!]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Scriptlets must be sane, if used.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[!]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Arch-ed rpms have a total of 6676480 bytes in /usr/share 10240
     rasmol-gtk-2.7.5-3.fc16.x86_64.rpm 5744640 rasmol-
     doc-2.7.5-3.fc16.x86_64.rpm 921600 rasmol-2.7.5-3.fc16.x86_64.rpm
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: rasmol-2.7.5-3.fc16.x86_64.rpm
          rasmol-gtk-2.7.5-3.fc16.x86_64.rpm
          rasmol-doc-2.7.5-3.fc16.x86_64.rpm
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 1: warning: macro `PU' not defined
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 4119: warning: macro `false',' not defined
rasmol.x86_64: E: incorrect-fsf-address /usr/share/doc/rasmol-2.7.5/GPL
rasmol.x86_64: W: no-manual-page-for-binary rasmolb
rasmol-gtk.x86_64: W: no-documentation
rasmol-gtk.x86_64: W: no-manual-page-for-binary grasmol
rasmol-doc.x86_64: W: file-not-utf8 /usr/share/doc/rasmol-doc-2.7.5/doc/itrasmol2721.hlp
3 packages and 0 specfiles checked; 1 errors, 6 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint rasmol-doc rasmol-gtk rasmol
rasmol-doc.x86_64: I: enchant-dictionary-not-found en_US
rasmol-doc.x86_64: W: file-not-utf8 /usr/share/doc/rasmol-doc-2.7.5/doc/itrasmol2721.hlp
rasmol-gtk.x86_64: W: no-documentation
rasmol-gtk.x86_64: W: no-manual-page-for-binary grasmol
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 1: warning: macro `PU' not defined
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 4119: warning: macro `false',' not defined
rasmol.x86_64: E: incorrect-fsf-address /usr/share/doc/rasmol-2.7.5/GPL
rasmol.x86_64: W: no-manual-page-for-binary rasmolb
3 packages and 0 specfiles checked; 1 errors, 6 warnings.
# echo 'rpmlint-done:'



Requires
--------
rasmol-doc (rpmlib, GLIBC filtered):
    rasmol

rasmol-gtk (rpmlib, GLIBC filtered):
    /bin/sh
    libCNearTree.so.5()(64bit)
    libCQRlib.so.2()(64bit)
    libCVector-1.0.3.so.2()(64bit)
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libXi.so.6()(64bit)
    libatk-1.0.so.0()(64bit)
    libc.so.6()(64bit)
    libcairo.so.2()(64bit)
    libcbf.so.0()(64bit)
    libfontconfig.so.1()(64bit)
    libfreetype.so.6()(64bit)
    libgdk-x11-2.0.so.0()(64bit)
    libgdk_pixbuf-2.0.so.0()(64bit)
    libgio-2.0.so.0()(64bit)
    libglib-2.0.so.0()(64bit)
    libgmodule-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgthread-2.0.so.0()(64bit)
    libgtk-x11-2.0.so.0()(64bit)
    libm.so.6()(64bit)
    libpango-1.0.so.0()(64bit)
    libpangocairo-1.0.so.0()(64bit)
    libpangoft2-1.0.so.0()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libvte.so.9()(64bit)
    rasmol
    rtld(GNU_HASH)

rasmol (rpmlib, GLIBC filtered):
    /bin/sh
    libCNearTree.so.5()(64bit)
    libCQRlib.so.2()(64bit)
    libCVector-1.0.3.so.2()(64bit)
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libXi.so.6()(64bit)
    libXpm.so.4()(64bit)
    libc.so.6()(64bit)
    libcbf.so.0()(64bit)
    libforms.so.2()(64bit)
    libm.so.6()(64bit)
    rtld(GNU_HASH)
    xterm



Provides
--------
rasmol-doc:
    rasmol-doc
    rasmol-doc(x86-64)

rasmol-gtk:
    rasmol-gtk
    rasmol-gtk(x86-64)

rasmol:
    rasmol
    rasmol(x86-64)



MD5-sum check
-------------
http://www.rasmol.org/software/RasMol_2.7.5.tar.gz :
  CHECKSUM(SHA256) this package     : aa7e7fd4b3c074e67697bd6adf7e86b9a4c60b605f5a319d6ecdd144b39f7fe9
  CHECKSUM(SHA256) upstream package : aa7e7fd4b3c074e67697bd6adf7e86b9a4c60b605f5a319d6ecdd144b39f7fe9


Generated by fedora-review 0.4.0 (660ce56) last change: 2013-01-29
Buildroot used: fedora-16-x86_64
Command line :/usr/bin/fedora-review -b 915144
Comment 7 Dmitrij S. Kryzhevich 2013-03-12 02:43:56 UTC 
Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-4.fc18.src.rpm


Thanks.

> - gzip can be omitted

Done.

> - Another license (LGPLv2+) has been detected for 'eggfileformatchooser.h'
> file. You should consider to add a multiple licensing:

Right. Missed that.

> - Patches link to upstream bugs/comments/lists or are otherwise justified.
> These info are missing, add them if possible.

Didn't catch that.
Add comment on pathes but bot sure it is what you mean.


> - Package uses nothing in %doc for runtime.
>   -doc sub-package contains '%{_datadir}/%{name}/data' directory with all
> *.pdb* files; .pdb files are Protein Data Bank data files used from
> application. 
> Why they are excluded from main package ? If they were not, -doc sub-package
> could be packaged as 'noarch' and could contain only document files.

noarch added.
data dir is a dir with examples, their are not needed for rasmol to work actualy.

> - Fully versioned dependency in subpackages, if present.
>   I don't know if Epochs is strictly necessary for rasmol; if no, 
>   'Requires: %{name}%{?_isa} = %{version}-%{release}' is better.

Why not? Fixed.

> - Package consistently uses macro is (instead of hard-coded directory names).
>   %%{name} macro can be used more frequently in .spec file.

May be. Not sure.
Hardcoded system dir names are not present.

> - rasmol-doc.x86_64: W: file-not-utf8
> /usr/share/doc/rasmol-doc-2.7.5/doc/itrasmol2721.hlp
> This warning seems can be fixed by using something like that:
> 
> iconv --from=ISO-8859-1 --to=UTF-8 doc/itrasmol2721.hlp >
> doc/itrasmol2721_new.hlp 
> mv doc/itrasmol2721_new.hlp doc/itrasmol2721.hlp

ISO-8859-1? How could I gues that...

> - update-desktop-database is invoked when required
>   Note: desktop file(s) in rasmol, rasmol-gtk
>   See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

Icon cache updated in rasmol package, no more icons for rasmol-gtk.
http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage was used for .desktop files.
Again, what is wrong?

> rasmol.x86_64: E: incorrect-fsf-address /usr/share/doc/rasmol-2.7.5/GPL

This is semipositive issue.
Comment 8 Antonio T. (sagitter) 2013-03-12 10:41:49 UTC 
(In reply to comment #7)
> 
> > - Package consistently uses macro is (instead of hard-coded directory names).
> >   %%{name} macro can be used more frequently in .spec file.
> 
> May be. Not sure.
> Hardcoded system dir names are not present.

'install -D -m 755 %{SOURCE4} %{buildroot}%{_bindir}/rasmol' becomes
'install -D -m 755 %{SOURCE4} %{buildroot}%{_bindir}/%{name}'

or 

'cp data/* %{buildroot}%{_datadir}/rasmol/data/' becomes 
'cp data/* %{buildroot}%{_datadir}/%{name}/data/'

or again

'Patch1:         rasmol-2.7.5-23Jul09-gtk.patch' becomes
'Patch1:         %{name}-2.7.5-23Jul09-gtk.patch'

> 
> > - update-desktop-database is invoked when required
> >   Note: desktop file(s) in rasmol, rasmol-gtk
> >   See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
> 
> Icon cache updated in rasmol package, no more icons for rasmol-gtk.
> http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-
> install_usage was used for .desktop files.
> Again, what is wrong?

Probably it refers to missing icon in gtk sub-package but indicated by grasmol.desktop file. I'm not sure.
Comment 9 Dmitrij S. Kryzhevich 2013-03-12 11:15:05 UTC 
(In reply to comment #8)
> 'install -D -m 755 %{SOURCE4} %{buildroot}%{_bindir}/rasmol' becomes
> 'install -D -m 755 %{SOURCE4} %{buildroot}%{_bindir}/%{name}'
> 
> or 
> 
> 'cp data/* %{buildroot}%{_datadir}/rasmol/data/' becomes 
> 'cp data/* %{buildroot}%{_datadir}/%{name}/data/'
> 
> or again
> 
> 'Patch1:         rasmol-2.7.5-23Jul09-gtk.patch' becomes
> 'Patch1:         %{name}-2.7.5-23Jul09-gtk.patch'

Let it be as is. Putting %{name} wherever it could be put - I think it is wrong.


> Probably it refers to missing icon in gtk sub-package but indicated by
> grasmol.desktop file. I'm not sure.

No icons in rasmol-gtk is that what suppose to be. It is not a realy issue I think.
Comment 10 Michael Schwendt 2013-03-12 11:46:51 UTC 
> 'Patch1:         rasmol-2.7.5-23Jul09-gtk.patch' becomes
> 'Patch1:         %{name}-2.7.5-23Jul09-gtk.patch'

Not necessarily a good idea to do that. These tags refer to files with a hardcoded file name, because they have been checked into git with those file names. If %name changes, the file names don't change automatically. But if the patches still apply afterwards, there is no need to rename them.


> - update-desktop-database is invoked when required

But is it required?

  $ grep -i mime *.desktop
  $

http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database


> Icon cache updated in rasmol package, no more icons for rasmol-gtk.

What does that mean? Have you added the scriptlets in release -4? If so, please mention such details in the %changelog. It isn't easy to follow this review when you mention changes in bugzilla, which don't match the %changelog. The previous src.rpm is gone already, too, and as I don't have a copy of it, rpmdiff cannot be used.

So, with regard to update-desktop-database and gtk-update-icon-cache I can only guess that there is some confusion.


> Requires: %{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> %package doc
> Requires: %{name} = %{version}-%{release}

Please don't add a base dep to -doc subpackages. Documentation packages ought to stay free of such dependencies. Imagine someone who wants to peruse the documentation without being forced to install the application (and possibly lots of dependencies).

In case you worry about ownership of %{_datadir}/%{name}/, having multiple packages own the same dir is okay with current packaging guidelines.
https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership


> # 64b version require one more definition
> %if "%{?_lib}" == "lib64"
> %patch2 -p1

Dangerous. This assumes that every 64-bit platform uses lib64, even if it may not support multilib.


> # Fix harcoded vars
> sed -i -e 's|PKGDIR = $(HOME)|PKGDIR = /usr|' -e 's|/usr/local|/usr|g' 
> -e 's|RASMOLDIR = $(USRLIBDIR)/rasmol/|RASMOLDIR = %{_datadir}/rasmol/|' \

Without checking each of these substitutions, please consider adding guards (e.g. with a grep before or afterwards). Alternatively, add a comment that explains whether the %files section guards against failing substitutions already. Sed substitutions are fragile. If a match fails, because the target file has changed, no substitution would be performed. That's a case that may cause the build to fail later (e.g. due to the non-matching %files section, which would be good) or succeed with installing files in wrong locations or building the software with wrong built-in paths (which would be bad).
Comment 11 Dmitrij S. Kryzhevich 2013-03-13 04:13:20 UTC 
Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-5.fc18.src.rpm


(In reply to comment #10)
> > Icon cache updated in rasmol package, no more icons for rasmol-gtk.
> 
> What does that mean? Have you added the scriptlets in release -4? If so,
> please mention such details in the %changelog.

I didn't. It was already in relese -1.


> So, with regard to update-desktop-database and gtk-update-icon-cache I can
> only guess that there is some confusion.

update-desktop-database is not required. gtk-update-icon-cache is used to for installed icon. That icon is installed in main package and no more icons are installed in subpackages so (as I thinks) subpackages does not require to call gtk-update-icon-cache. Both main and -gtk subpackage call update-desktop-database as both of them have .desktop files.

> > Requires: %{name} = %{version}-%{release}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

%{?_isa} added.


> > %package doc
> > Requires: %{name} = %{version}-%{release}
> 
> Please don't add a base dep to -doc subpackages. Documentation packages
> ought to stay free of such dependencies. Imagine someone who wants to peruse
> the documentation without being forced to install the application (and
> possibly lots of dependencies).

I think you are right. Removed. %files fixed.


> > # 64b version require one more definition
> > %if "%{?_lib}" == "lib64"
> > %patch2 -p1
> 
> Dangerous. This assumes that every 64-bit platform uses lib64, even if it
> may not support multilib.

Example was taken from crossplatform cmake package which runs on at least ppc and arm. Both put 64b libs in /usr/lib64, right. But in Fedora at all main platforms 64b libs are in /usr/lib64. Sorry, I can't handle all possibilites. I just suppose that %{?_lib} is not an arch (as 'ia64' and 'ppc64' are not 'lib64' obviously) so it would be work correctly anywhere.
But moreover it assumes that every 64-bit platform requires that definition in patch. Documentation say it is required. I could test only x64_64/i686 and not even arm.


> > # Fix harcoded vars
> > sed -i -e 's|PKGDIR = $(HOME)|PKGDIR = /usr|' -e 's|/usr/local|/usr|g' 
> > -e 's|RASMOLDIR = $(USRLIBDIR)/rasmol/|RASMOLDIR = %{_datadir}/rasmol/|' \
> 
> Without checking each of these substitutions, please consider adding guards
> (e.g. with a grep before or afterwards). Alternatively, add a comment that
> explains whether the %files section guards against failing substitutions
> already. Sed substitutions are fragile. If a match fails, because the target
> file has changed, no substitution would be performed. That's a case that may
> cause the build to fail later (e.g. due to the non-matching %files section,
> which would be good) or succeed with installing files in wrong locations or
> building the software with wrong built-in paths (which would be bad).

1) All substitutions beside RASMOLDIR. It is what could brake compilation and compilation only.
2) RASMOLDIR. It is handle where place PDB and help files. If they (files) whould be in some other place, /usr/lib/rpm/check-files will provide an error.

That comment was placed in spec.

%changelog
* Thu Mar 13 2013 Dmitrij S. Kryzhevich <krege> 2.7.5-5
- Dependency for -gtk subpackage use ?_isa macros.
- Drop main package dependency for -doc subpackage.
- Fix files section for -doc according new deps.
- sed substitutions were commented.
- Fix previouse changelog (added last string).

* Mon Mar 04 2013 Dmitrij S. Kryzhevich <krege> 2.7.5-4
- doc subpackage is noarch now.
- Some cleanups.
- Fix encoding for doc/itrasmol2721.hlp.
Comment 12 Dmitrij S. Kryzhevich 2013-03-13 04:26:48 UTC 
> I just suppose that %{?_lib} is not an arch (as 'ia64' and
> 'ppc64' are not 'lib64' obviously) so it would be work correctly anywhere.

And one more: qt package check paths in the same way. So it looks to be safe.
Comment 13 Michael Schwendt 2013-03-13 10:09:54 UTC 
> update-desktop-database is not required. 

> Both main and -gtk subpackage call update-desktop-database
> as both of them have .desktop files.

That's contradictory. So, you run the tool although that's not required? That's harmless, but superfluous.


> qt package check paths in the same way. So it looks to be safe.

Qt does it to adjust install paths. You do it to patch a header file that influences compilation.
Comment 14 Dmitrij S. Kryzhevich 2013-03-13 12:13:07 UTC 
Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-6.fc18.src.rpm

(In reply to comment #13)
> > update-desktop-database is not required. 
> 
> > Both main and -gtk subpackage call update-desktop-database
> > as both of them have .desktop files.
> 
> That's contradictory. So, you run the tool although that's not required?
> That's harmless, but superfluous.

Oh. What a... Hm. Shame for me. It is only now when I understand what you both talking about. Sorry. Sure, it is not required and they are removed.

> > qt package check paths in the same way. So it looks to be safe.
> 
> Qt does it to adjust install paths. You do it to patch a header file that
> influences compilation.

But the check itself for paths looks to be the same.
Anyway, are there better ideas?

* Thu Mar 13 2013 Dmitrij S. Kryzhevich <krege> 2.7.5-6
- Remove update-desktop-database from post and postun scripts as .desktop files do not contain MimeType key.
Comment 15 Michael Schwendt 2013-03-13 14:08:37 UTC 
How about evaluating %__isa_bits instead?

%if "%{?__isa_bits}" == "64"
  # ...
%else
  # ...
%endif
Comment 16 Dmitrij S. Kryzhevich 2013-03-14 02:37:42 UTC 
Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-7.fc18.src.rpm

(In reply to comment #15)
> How about evaluating %__isa_bits instead?
> 
> %if "%{?__isa_bits}" == "64"
>   # ...
> %else
>   # ...
> %endif

Had found that in qt.spec, didn't have time to update yestersay.

* Wed Mar 14 2013 Dmitrij S. Kryzhevich <krege> 2.7.5-7
- Let apply 64b patch according to the __isa_bits macros.
Comment 17 Dmitrij S. Kryzhevich 2013-05-30 06:23:23 UTC 
Michael, could you (and wish) performe the review?
Comment 18 Björn 'besser82' Esser 2013-06-21 13:16:10 UTC 
Package has issues, see below.

#####

Package Review
==============

Legend:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- update-desktop-database is invoked when required
  Note: desktop file(s) in rasmol, rasmol-gtk
  See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

  ---> false positive: no mime-type in desktop-file

- bogus date in %changelog: Wed Mar 14 2013 Dmitrij S. Kryzhevich 2.7.5-7
- bogus date in %changelog: Thu Mar 13 2013 Dmitrij S. Kryzhevich 2.7.5-6
- bogus date in %changelog: Thu Mar 13 2013 Dmitrij S. Kryzhevich 2.7.5-5

  ---> fix this, please

- manual-page-warning rasmol.1x.gz 1: warning: macro `PU' not defined
- manual-page-warning rasmol.1x.gz 4119: warning: macro `false',' not defined

  ---> please fix this, e.g. with patch

- rasmol.x86_64: E: incorrect-fsf-address /usr/share/doc/rasmol-2.7.5/GPL

  ---> please inform upstream and ask for including recent rev.
       of that document

- more issues to be found in inline-comments of report


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: %build honors applicable compiler flags or justifies otherwise.

     ---> LDFLAGS are ommitted on linking

[x]: Package contains no bundled libraries without FPC exception.
[!]: Changelog in prescribed format.

     ---> bogus dates, see above

[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Fully versioned dependency in subpackages, if present.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rasmol-doc

     ---> false positve: doc-subpkg is noarch and should not require binaries

[x]: Package complies to the Packaging Guidelines
[x]: 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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "LGPL (v2 or later) (with incorrect FSF address)", "Unknown or
     generated". 97 files have unknown license. Detailed output of
     licensecheck in
     /home/bjoern.esser/fedora/review/915144-rasmol/licensecheck.txt
[!]: License file installed when any subpackage combination is installed.

     ---> add %doc GPL to -doc-pkg

[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: gtk-update-icon-cache is invoked when required
     Note: icons in rasmol
[x]: Useful -debuginfo package or justification otherwise.
[!]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 481280 bytes in 20 files.

     ---> you should move all %doc, but GPL, to doc-pkg

[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[-]: 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]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.

     ---> no check-target avail in Makefile

[!]: Packages should try to preserve timestamps of original installed files.

     ---> add -p switch to install-commands, please.

[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: rasmol-2.7.5-7.fc20.x86_64.rpm
          rasmol-gtk-2.7.5-7.fc20.x86_64.rpm
          rasmol-doc-2.7.5-7.fc20.noarch.rpm
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 1: warning: macro `PU' not defined
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 4119: warning: macro `false',' not defined
rasmol.x86_64: E: incorrect-fsf-address /usr/share/doc/rasmol-2.7.5/GPL
rasmol.x86_64: W: no-manual-page-for-binary rasmolb
rasmol-gtk.x86_64: W: no-documentation
rasmol-gtk.x86_64: W: no-manual-page-for-binary grasmol
3 packages and 0 specfiles checked; 1 errors, 5 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint rasmol-doc rasmol-gtk rasmol
rasmol-gtk.x86_64: W: no-documentation
rasmol-gtk.x86_64: W: no-manual-page-for-binary grasmol
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 1: warning: macro `PU' not defined
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 4119: warning: macro `false',' not defined
rasmol.x86_64: E: incorrect-fsf-address /usr/share/doc/rasmol-2.7.5/GPL
rasmol.x86_64: W: no-manual-page-for-binary rasmolb
3 packages and 0 specfiles checked; 1 errors, 5 warnings.
# echo 'rpmlint-done:'



Requires
--------
rasmol-doc (rpmlib, GLIBC filtered):

rasmol-gtk (rpmlib, GLIBC filtered):
    libCNearTree.so.5()(64bit)
    libCQRlib.so.2()(64bit)
    libCVector-1.0.3.so.2()(64bit)
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libXi.so.6()(64bit)
    libatk-1.0.so.0()(64bit)
    libc.so.6()(64bit)
    libcairo.so.2()(64bit)
    libcbf.so.0()(64bit)
    libfontconfig.so.1()(64bit)
    libfreetype.so.6()(64bit)
    libgdk-x11-2.0.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-x11-2.0.so.0()(64bit)
    libm.so.6()(64bit)
    libpango-1.0.so.0()(64bit)
    libpangocairo-1.0.so.0()(64bit)
    libpangoft2-1.0.so.0()(64bit)
    libvte.so.9()(64bit)
    rasmol(x86-64)
    rtld(GNU_HASH)

rasmol (rpmlib, GLIBC filtered):
    /bin/sh
    libCNearTree.so.5()(64bit)
    libCQRlib.so.2()(64bit)
    libCVector-1.0.3.so.2()(64bit)
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libXi.so.6()(64bit)
    libXpm.so.4()(64bit)
    libc.so.6()(64bit)
    libcbf.so.0()(64bit)
    libforms.so.2()(64bit)
    libm.so.6()(64bit)
    rtld(GNU_HASH)
    xterm



Provides
--------
rasmol-doc:
    rasmol-doc

rasmol-gtk:
    rasmol-gtk
    rasmol-gtk(x86-64)

rasmol:
    rasmol
    rasmol(x86-64)



Source checksums
----------------
http://www.rasmol.org/software/RasMol_2.7.5.tar.gz :
  CHECKSUM(SHA256) this package     : aa7e7fd4b3c074e67697bd6adf7e86b9a4c60b605f5a319d6ecdd144b39f7fe9
  CHECKSUM(SHA256) upstream package : aa7e7fd4b3c074e67697bd6adf7e86b9a4c60b605f5a319d6ecdd144b39f7fe9


Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 915144

#####

Please fix the issues and I'll take another review.
Comment 19 Dmitrij S. Kryzhevich 2013-06-28 04:31:32 UTC 
Sorry for silence, I'l be ready for work with rasmol at about 10th of July.
Comment 20 Björn 'besser82' Esser 2013-07-19 13:12:57 UTC 
Any new progress here?
Comment 21 Dmitrij S. Kryzhevich 2013-07-22 08:17:16 UTC 
Sorry for the delay, again.

(In reply to Björn Esser from comment #18)
> - bogus date in %changelog: Wed Mar 14 2013 Dmitrij S. Kryzhevich 2.7.5-7
> - bogus date in %changelog: Thu Mar 13 2013 Dmitrij S. Kryzhevich 2.7.5-6
> - bogus date in %changelog: Thu Mar 13 2013 Dmitrij S. Kryzhevich 2.7.5-5
> 
>   ---> fix this, please

Done.

> - manual-page-warning rasmol.1x.gz 1: warning: macro `PU' not defined
> - manual-page-warning rasmol.1x.gz 4119: warning: macro `false',' not defined
> 
>   ---> please fix this, e.g. with patch

I'm trying to understang what is called one after another in Makefile. rasmol tar contain formated file AND man file AND compressed man file.

> - rasmol.x86_64: E: incorrect-fsf-address /usr/share/doc/rasmol-2.7.5/GPL
> 
>   ---> please inform upstream and ask for including recent rev.
>        of that document

Done (it was done before).

>      ---> LDFLAGS are ommitted on linking

Done.

> [!]: License file installed when any subpackage combination is installed.
> 
>      ---> add %doc GPL to -doc-pkg

Done.

> [!]: Large documentation must go in a -doc subpackage.
>      Note: Documentation size is 481280 bytes in 20 files.
> 
>      ---> you should move all %doc, but GPL, to doc-pkg

Hm... Changelog and readme? All doc is already in -doc. What do you mean?


> [!]: Packages should try to preserve timestamps of original installed files.
> 
>      ---> add -p switch to install-commands, please.

Done.

Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-8.fc18.src.rpm
Comment 22 Dmitrij S. Kryzhevich 2013-07-22 08:18:42 UTC 
Ah, this version contain fix for man file too, it is fixed. That was old remark.
Comment 23 Dmitrij S. Kryzhevich 2013-07-26 03:59:49 UTC 
Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-9.fc19.src.rpm

%changelog
* Thu Jul 25 2013 Dmitrij S. Kryzhevich 2.7.5-9
- Some missed -p key for install command.
- Added missed entry to the prev changelog section (man file).

* Mon Jul 22 2013 Dmitrij S. Kryzhevich 2.7.5-8
- Fixed bogus dates.
- Added GPL file to the -doc subpackage.
- Added -p key to "install" command.
- Fixed man file. Added patch for it.
Comment 24 Christopher Meng 2013-07-26 04:05:18 UTC 
I think you don't need to include a file's 3 different types.

See %doc

%doc ChangeLog
%doc ChangeLog.html ChangeLog.txt GPL NOTICE PROJECTS RASLIC README.html README.txt TODO.html TODO.txt history.html

Please remove redundant files.
Comment 25 Dmitrij S. Kryzhevich 2013-07-26 05:58:19 UTC 
(In reply to Christopher Meng from comment #24)
What are 3 types? I see only 2.
Which type to remove, txt or html?
Comment 26 Christopher Meng 2013-07-26 06:06:20 UTC 
Not sure.

Waiting for reviewer.
Comment 27 Dmitrij S. Kryzhevich 2013-07-26 06:08:10 UTC 
If you don't mind, I would like to left both of them.
Comment 28 Michael Schwendt 2013-07-26 07:56:56 UTC 
> Which type to remove, txt or html?

Neither one. ;-p  Keep'em all. Nothing wrong with that.
Comment 29 Dmitrij S. Kryzhevich 2013-07-31 09:01:56 UTC 
Ping?
Comment 30 Dmitrij S. Kryzhevich 2013-09-09 05:25:38 UTC 
Ping ^2 ?
Comment 31 Björn 'besser82' Esser 2013-09-11 12:34:37 UTC 
Sorry for the huge delay here.  Your package has some small issues left.

#####

Package Review
==============

Legend:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: 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]: License field in the package spec file matches the actual license.

     ---> License is fine.

[!]: License file installed when any subpackage combination is installed.

     ---> the -doc pkg misses some of the license-files.  In fact the files
          named NOTICE and RASLIC carry some imported notes on license, too.
          

[x]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/icons/hicolor,
     /usr/share/icons/hicolor/48x48, /usr/share/icons/hicolor/48x48/apps

     ---> your package MUST Requires: hicolor-icon-theme

[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: gtk-update-icon-cache is invoked in %postun and %posttrans if package
     contains icons.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[!]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 481280 bytes in 20 files.

     ---> the ChangeLog-folder just contains split copies of ChangeLog.txt,
          so you can purge it from being included.  The whole %doc in the
          arched pkg is about the same size as the binaries within.  I'd
          recommend to move all, but GPL, NOTICE, RASLIC, to the -doc pkg.
          You will need to include NOTICE and RASLIC into the -doc, too,
          because these files carry some important informations about
          licensing/copyright as well.

[!]: Package complies to the Packaging Guidelines

     ---> issues are present

[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or desktop-
     file-validate if there is such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: 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]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: rasmol-2.7.5-9.fc21.x86_64.rpm
          rasmol-gtk-2.7.5-9.fc21.x86_64.rpm
          rasmol-doc-2.7.5-9.fc21.noarch.rpm
          rasmol-2.7.5-9.fc21.src.rpm
rasmol.x86_64: E: incorrect-fsf-address /usr/share/doc/rasmol/GPL
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 4118: warning: macro `false',' not defined
rasmol.x86_64: W: no-manual-page-for-binary rasmolb
rasmol-gtk.x86_64: W: no-documentation
rasmol-gtk.x86_64: W: no-manual-page-for-binary grasmol
rasmol-doc.noarch: E: incorrect-fsf-address /usr/share/doc/rasmol-doc/GPL
4 packages and 0 specfiles checked; 2 errors, 4 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint rasmol-doc rasmol-gtk rasmol
rasmol-doc.noarch: E: incorrect-fsf-address /usr/share/doc/rasmol-doc/GPL
rasmol-gtk.x86_64: W: no-documentation
rasmol-gtk.x86_64: W: no-manual-page-for-binary grasmol
rasmol.x86_64: E: incorrect-fsf-address /usr/share/doc/rasmol/GPL
rasmol.x86_64: W: manual-page-warning /usr/share/man/man1/rasmol.1x.gz 4118: warning: macro `false',' not defined
rasmol.x86_64: W: no-manual-page-for-binary rasmolb
3 packages and 0 specfiles checked; 2 errors, 4 warnings.
# echo 'rpmlint-done:'



Requires
--------
rasmol-doc (rpmlib, GLIBC filtered):

rasmol-gtk (rpmlib, GLIBC filtered):
    libCNearTree.so.5()(64bit)
    libCQRlib.so.2()(64bit)
    libCVector-1.0.3.so.2()(64bit)
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libXi.so.6()(64bit)
    libatk-1.0.so.0()(64bit)
    libc.so.6()(64bit)
    libcairo.so.2()(64bit)
    libcbf.so.0()(64bit)
    libfontconfig.so.1()(64bit)
    libfreetype.so.6()(64bit)
    libgdk-x11-2.0.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-x11-2.0.so.0()(64bit)
    libm.so.6()(64bit)
    libpango-1.0.so.0()(64bit)
    libpangocairo-1.0.so.0()(64bit)
    libpangoft2-1.0.so.0()(64bit)
    libvte.so.9()(64bit)
    rasmol(x86-64)
    rtld(GNU_HASH)

rasmol (rpmlib, GLIBC filtered):
    /bin/sh
    libCNearTree.so.5()(64bit)
    libCQRlib.so.2()(64bit)
    libCVector-1.0.3.so.2()(64bit)
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libXi.so.6()(64bit)
    libXpm.so.4()(64bit)
    libc.so.6()(64bit)
    libcbf.so.0()(64bit)
    libforms.so.2()(64bit)
    libm.so.6()(64bit)
    rtld(GNU_HASH)
    xterm



Provides
--------
rasmol-doc:
    rasmol-doc

rasmol-gtk:
    rasmol-gtk
    rasmol-gtk(x86-64)

rasmol:
    rasmol
    rasmol(x86-64)



Source checksums
----------------
http://www.rasmol.org/software/RasMol_2.7.5.tar.gz :
  CHECKSUM(SHA256) this package     : aa7e7fd4b3c074e67697bd6adf7e86b9a4c60b605f5a319d6ecdd144b39f7fe9
  CHECKSUM(SHA256) upstream package : aa7e7fd4b3c074e67697bd6adf7e86b9a4c60b605f5a319d6ecdd144b39f7fe9


Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 915144
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, SugarActivity, Perl, R, PHP, Ruby
Disabled flags: EPEL5, EXARCH, DISTTAG

#####

Resolve these last small four issues and I'll grant review.  :)
Comment 32 Dmitrij S. Kryzhevich 2013-09-12 08:33:18 UTC 
(In reply to Björn "besser82" Esser from comment #31)
>      ---> the -doc pkg misses some of the license-files.  In fact the files
>           named NOTICE and RASLIC carry some imported notes on license, too.

But it does:
/usr/share/doc/rasmol-doc-2.7.5/doc/RASLIC
/usr/share/doc/rasmol-doc-2.7.5/doc/NOTICE.html

>      ---> your package MUST Requires: hicolor-icon-theme

Fixed.


> [!]: Large documentation must go in a -doc subpackage. Large could be size
>      (~1MB) or number of files.
>      Note: Documentation size is 481280 bytes in 20 files.
> 
>      ---> the ChangeLog-folder just contains split copies of ChangeLog.txt,
>           so you can purge it from being included.  The whole %doc in the
>           arched pkg is about the same size as the binaries within.  I'd
>           recommend to move all, but GPL, NOTICE, RASLIC, to the -doc pkg.
>           You will need to include NOTICE and RASLIC into the -doc, too,
>           because these files carry some important informations about
>           licensing/copyright as well.

ChangeLog-folder contain splitted old changelog from 1993 with a file (ChangeLog.9) wich is a copy of ChangeLog.txt. So, it is ChangeLog.txt what is not required, not dir. I would like to left README.txt as well as the Changelog dir. Everything else beside GPL, NOTICE and RASLIC goes to -doc.


What is something to worry about is a change in %doc macros for F20, did not tested that yet.

Spec URL: http://krege.fedorapeople.org/rasmol/rasmol.spec
SRPM URL: http://krege.fedorapeople.org/rasmol/rasmol-2.7.5-10.fc19.src.rpm

%changelog
* Thu Sep 12 2013 Dmitrij S. Kryzhevich 2.7.5-10
- Fixed dir ownership.
- Move some files from main package to -doc.
Comment 33 Dmitrij S. Kryzhevich 2013-09-12 08:48:33 UTC 
(In reply to Dmitrij S. Kryzhevich from comment #32)
 
> What is something to worry about is a change in %doc macros for F20, did not
> tested that yet.

And I tested. It works.
Comment 34 Björn 'besser82' Esser 2013-09-12 11:46:34 UTC 
LGTM, now  :)

APPROVED!!!
Comment 35 Dmitrij S. Kryzhevich 2013-09-13 08:48:21 UTC 
New Package SCM Request
=======================
Package Name: rasmol
Short Description: Molecular graphics visualization tool
Owners: krege
Branches: f18 f19 f20
InitialCC:


Thanks everybody!
Comment 36 Gwyn Ciesla 2013-09-13 13:10:53 UTC 
Git done (by process-git-requests).
Comment 37 Fedora Update System 2013-09-14 06:11:13 UTC 
rasmol-2.7.5-10.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/rasmol-2.7.5-10.fc20
Comment 38 Dmitrij S. Kryzhevich 2013-09-14 06:14:05 UTC 
See also https://admin.fedoraproject.org/updates/rasmol-2.7.5-10.fc19 and https://admin.fedoraproject.org/updates/rasmol-2.7.5-10.fc18 for corresponding submittings.
Comment 39 Fedora Update System 2013-09-14 17:45:14 UTC 
rasmol-2.7.5-10.fc20 has been pushed to the Fedora 20 testing repository.
Comment 40 Fedora Update System 2013-09-24 00:23:39 UTC 
rasmol-2.7.5-10.fc20 has been pushed to the Fedora 20 stable repository.