Bug 226321

This is one of the worst specs I have ever seen, so I decided to review this. Stay tuned.

First, let's go through the spec step by step:

Summary: No dot at the end please

name: Capital letter at the beginning

Prereq: should not be used, I doubt it really is one. sgml-common is a Requires and BuildRequires.

Is the license tag really correct? I could not find anything in the sources.

URL tag is missing

BuildRoot: is invalid, should be 
  %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
see https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

Prefix: /usr: drop this completely. Will be set automatically and using absolute paths is strictly forbidden

Requires: emacs is missing

%define psgmldir %{prefix}/share/emacs/site-lisp/psgml/: use %global, see
https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define
%{prefix}/share/ should be %{_datadir} here

%description: line breaks at 80 characters 


%build:

PATH=/usr/bin:$PATH this is most likely unnecessary

./configure --prefix=/usr: Hardcoded paths are strictly forbidden, just use %configure here.

Use parallel make if possible. If not, write a comment, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

During build I see a lot of errors like this one:
  Warning: !! The file uses old-style backquotes !!
  This functionality has been obsolete for more than 10 years already
  and will be removed soon.  See (elisp)Backquote in the manual.
Please look into this.

%install:

When creating psgml-init.el you should also use macros, %{psgmldir} instead of 
/usr/share/emacs/site-lisp/psgml, %{_sysconfdir} instead of /etc and so on

mkdir -p $RPM_BUILD_ROOT/%{prefix}/share/sgml/cdtd: use %{_datadir} instead of %{prefix}/share

Timestamps are not preserved during install, add INSTALL="install -p"

The install-info scriptlets lack the "|| :" at the end of the line and the %postun scriptlet is wrong, should be run in %preun, see
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Texinfo


%files:

%defattr should be (-,root,root,-)

%{prefix}/share/sgml/cdtd should be %{_datadir}/sgml/cdtd

ChangeLog and psgml.texi are missing from %doc

ChangeLog, psgml.texi and psgml.info.gz are not UTF-8, please convert them as described in https://fedoraproject.org/wiki/PackageMaintainers/PackagingTricks#Convert_encoding_to_UTF-8



Now the full checklist. All FIX or ??? items are explained above.

FIX - MUST: rpmlint
$ rpmlint noarch/psgml-1.2.5-9.fc12.noarch.rpm 
psgml.noarch: W: summary-ended-with-dot A GNU Emacs major mode for editing SGML documents.
psgml.noarch: E: tag-not-utf8 %changelog
psgml.noarch: W: no-url-tag
psgml.noarch: W: file-not-utf8 /usr/share/info/psgml.info.gz
1 packages and 0 specfiles checked; 1 errors, 3 warnings.
OK - MUST: The package is named according to the Package Naming Guidelines.
OK - MUST: The spec file name matches the base package %{name}, in the format %{name}.spec.
FIX - MUST: The package meets the Packaging Guidelines.
OK - MUST: The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
??? - MUST: The License field in the package spec file matches the actual license.
FIX - MUST: The license file from the source package is included in %doc.
OK - MUST: The spec file is in American English.
OK - MUST: The spec file for the package is legible.
OK - MUST: The sources used to build the package match the upstream source by MD5 d4f346b0242035e54860b29d7466b0a2
OK - MUST: The package successfully compiles and builds into binary rpms on i386 (noarch)
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: All build dependencies are listed in BuildRequires.
N/A - MUST: The spec file handles locales properly with the %find_lang macro.
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
OK - MUST: The package owns all directories that it creates.
OK - MUST: The package does not contain any duplicate files in the %files listing.
OK - MUST: Permissions on files are set properly. Every %files section includes a %defattr(...) line.
OK - MUST: The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
FIX - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines.
OK - MUST: The package contains code, or permissable content.
N/A - MUST: Large documentation files should go in a -doc subpackage.
OK - MUST: Files included as %doc do not affect the runtime of the application.
N/A - MUST: Header files must be in a -devel package.
N/A - MUST: Static libraries must be in a -static package.
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
N/A - MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
N/A - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
OK - MUST: The package does not contain any .la libtool archives.
N/A - MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section.
OK - MUST: The packages does not own files or directories already owned by other packages.
OK - MUST: At the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT.
OK - MUST: All filenames in rpm packages are valid UTF-8.


SHOULD Items:
??? - 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.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: The the package builds in mock.
OK - SHOULD: The package should compile and build into binary rpms on all supported architectures.
OK - SHOULD: The package functions as described.
FIX - SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg.
N/A - SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.

Alex, you are listed as the owner of psgml, but you were not CC'ed to this bug. Can you please look at the review and incorporate my corrections?

Adam, would be nice if you replied, then I would have not needed 2 months to find out your are no longer the maintainer of psgml.

(In reply to comment #3)
> Alex, you are listed as the owner of psgml, but you were not CC'ed to this bug.
> Can you please look at the review and incorporate my corrections?

I took it to make sure it wasn't orphaned, but I haven't had time to work on it for a while and may not have much time for this for a week or so.  Patches would help speed it... ;)

Ultimately this should probably be renamed as emacs-sgml as per the review guidelines, but I'll incorporate your changes first.

Any news here?

(In reply to comment #5)
> Any news here?    

I'll try to look at it this week.

(In reply to comment #2)

New build here:

http://koji.fedoraproject.org/koji/buildinfo?buildID=155021

Diff between new spec and old spec in CVS:

http://cvs.fedoraproject.org/viewvc/rpms/psgml/devel/psgml.spec?r1=1.24&r2=1.25

> Summary: No dot at the end please

Fixed.

> name: Capital letter at the beginning

Fixed.

> Prereq: should not be used, I doubt it really is one. sgml-common is a Requires and BuildRequires.

Fixed.

> Is the license tag really correct? I could not find anything in the sources.
 
> URL tag is missing

Added.
 
> BuildRoot: is invalid, should be 
>   %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
> see https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

Fixed.

> Prefix: /usr: drop this completely. Will be set automatically and using
> absolute paths is strictly forbidden

Dropped.

> Requires: emacs is missing

Added.

> %define psgmldir %{prefix}/share/emacs/site-lisp/psgml/: use %global, see
> https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define
> %{prefix}/share/ should be %{_datadir} here

Fixed.

> %description: line breaks at 80 characters 

Line breaking was at 70 chars, now at 80 chars.


> %build:
 
> PATH=/usr/bin:$PATH this is most likely unnecessary

Removed.

> ./configure --prefix=/usr: Hardcoded paths are strictly forbidden, just use
> %configure here.

Done.
 
> Use parallel make if possible. If not, write a comment, see
> https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make
> 
> During build I see a lot of errors like this one:
>   Warning: !! The file uses old-style backquotes !!
>   This functionality has been obsolete for more than 10 years already
>   and will be removed soon.  See (elisp)Backquote in the manual.
> Please look into this.

psgml is a very old and currently unmaintained upstream AFAIK.  I will look into whether it's possible to easily fix this in a later revision.

> %install:
> 
> When creating psgml-init.el you should also use macros, %{psgmldir} instead of 
> /usr/share/emacs/site-lisp/psgml, %{_sysconfdir} instead of /etc and so on
> 
> mkdir -p $RPM_BUILD_ROOT/%{prefix}/share/sgml/cdtd: use %{_datadir} instead of
> %{prefix}/share

Used %{_datadir} and %{_sysconfdir} throughout .spec

> Timestamps are not preserved during install, add INSTALL="install -p"

> The install-info scriptlets lack the "|| :" at the end of the line and the
> %postun scriptlet is wrong, should be run in %preun, see
> https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Texinfo

Added "|| :" and made postun script a preun script as per above link.
 
> %files:
 
> %defattr should be (-,root,root,-)

Fixed

> %{prefix}/share/sgml/cdtd should be %{_datadir}/sgml/cdtd

Fixed.
 
> ChangeLog and psgml.texi are missing from %doc

Added.

> ChangeLog, psgml.texi and psgml.info.gz are not UTF-8, please convert them as
> described in
> https://fedoraproject.org/wiki/PackageMaintainers/PackagingTricks#Convert_encoding_to_UTF-8

Fixed.
 

> Now the full checklist. All FIX or ??? items are explained above.
> 
> FIX - MUST: rpmlint
> $ rpmlint noarch/psgml-1.2.5-9.fc12.noarch.rpm 
> psgml.noarch: W: summary-ended-with-dot A GNU Emacs major mode for editing SGML
> documents.
> psgml.noarch: E: tag-not-utf8 %changelog
> psgml.noarch: W: no-url-tag
> psgml.noarch: W: file-not-utf8 /usr/share/info/psgml.info.gz
> 1 packages and 0 specfiles checked; 1 errors, 3 warnings.

New rpmlint run:

rpmlint noarch/psgml-1.2.5-12.fc13.noarch.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.


> FIX - MUST: The package meets the Packaging Guidelines.

Should be fixed now.

> ??? - MUST: The License field in the package spec file matches the actual
> license.

Headers in .el files contain "GPLv2+" license, e.g.:

;; This program is free software; you can redistribute it and/or
;; modify it under the terms of the GNU General Public License
;; as published by the Free Software Foundation; either version 2
;; of the License, or (at your option) any later version.

which is GPLv2+ AFAIK.

> FIX - MUST: The license file from the source package is included in %doc.

Upstream didn't include separate COPYING file, not strictly necessary and since upstream is dead, not much point in filing a bug there.
.
> FIX - MUST: The package consistently uses macros, as described in the macros
> section of Packaging Guidelines.

I think this should be fixed now.

> SHOULD Items:
> ??? - 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.

See above.

> FIX - SHOULD: If scriptlets are used, those scriptlets must be sane. This is
> vague, and left up to the reviewers judgement to determine sanity.

Should be fixed now.

This should have been closed ages ago I think. You have addressed all problems and the package therefor is APPROVED.