Bug 812659

Here's what rpmlint says:

par.i386: W: spelling-error Summary(en_US) reformatter -> reformatted, reformatting, reformatory
par.i386: W: spelling-error Summary(en_US) fmt -> ft, mt, fit
par.i386: W: spelling-error %description -l en_US bodiless -> bodices
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

Not a review, just two comments:

File conflict: /usr/bin/par is also included in the par2cmdline package:
http://fedoraproject.org/wiki/Packaging:Conflicts#Binary_Name_Conflicts

Drop -s from install: http://fedoraproject.org/wiki/Packaging:Debuginfo

Spec URL: http://www.cs.wustl.edu/~levine/par/par-1.52-3.spec
SRPM URL: http://www.cs.wustl.edu/~levine/par/par-1.52-3.src.rpm

1) Renamed binary to /usr/bin/parr, man page to /usr/share/man/man1/parr.1.gz.
   (Did not update contents of the man page itself, pending resolution of
   conflict.)
2) Removed -s from install.


rpmlint output:

par.i386: W: spelling-error Summary(en_US) reformatter -> reformatted, reformatting, reformatory
par.i386: W: spelling-error Summary(en_US) fmt -> ft, mt, fit
par.i386: W: spelling-error %description -l en_US bodiless -> bodices
par.i386: W: unstripped-binary-or-object /usr/bin/parr
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

Just a brief look at the spec file:

* https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
* https://fedoraproject.org/wiki/Packaging/Guidelines
* https://fedoraproject.org/wiki/Packaging:NamingGuidelines

In particular, there are these for your reading pleasure: ;)

* https://fedoraproject.org/wiki/Packaging/SourceURL
* https://fedoraproject.org/wiki/Packaging/Guidelines#Tags
* https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
* https://fedoraproject.org/wiki/Packaging/Guidelines#Macros
* https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
* https://fedoraproject.org/wiki/Packaging/Guidelines#File_Permissions
* https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Using_the_.25.7B.3Fdist.7D_Tag

Okay, here are my initial set of comments:

* The following tags are obsolete and need to be removed:
Vendor, Packager

* The following tags are obsolete unless you plan on building for EPEL 5 or older:
- BuildRoot

* The following items are obsolete unless you plan on building for EPEL 5 or older:
- "rm -rf $RPM_BUILD_ROOT" at the beginning of %install
- a default %clean section that just contains "rm -rf $RPM_BUILD_ROOT"
- %defattr(-,root,root,755)

* I strongly advise that you always use macros like "%{buildroot}" instead of "%buildroot". While it isn't a huge deal in this relatively simple package, later, you may want to conditionalize macros (e.g. "%{?dist}"), and you will need to be using that syntax to do that cleanly.

* Please use "Source0" instead of "Source". Source0 should be a full URL to the upstream provided source tarball, rpm is smart enough to parse that. (Same is true for Patch1 since upstream provides it)

* In the Source0 URL, please use "%{name}" and "%{version}", so that as you update the package, that URL can remain relatively static (also prevents you from accidentally building a package with old source).

* In Release, please seriously consider using "%{?dist}" at the end of the numeric field. https://fedoraproject.org/wiki/Packaging:DistTag

* This code actually has a new license, instead of "Copyright only", please use:

License: Par

****
Clean all that up, and I'll come through and do a review.

Spec URL: http://www.cs.wustl.edu/~levine/par/par-1.52-4.spec
SRPM URL: http://www.cs.wustl.edu/~levine/par/par-1.52-4.fc16.src.rpm

Thank you both for your comments.  I tried to address all, except:

1) I retained the %defattr(-,root,root,755) for now.  Without it, /usr/share/doc/par-1.52/ is created with mode 2755.  rpmlint considers that an error:

  par.i386: E: non-standard-dir-perm /usr/share/doc/par-1.52 02755L

2) I didn't use macros in the Source0 URL because the name and version parts of its filename don't match the macro values (Par vs. par and 152 vs. 1.52).

3) And I restored the -s in the install of the binary.  The vast majority of binaries on my vanilla Fedora 16 are stripped, so I'm not sure why this one shouldn't be.  (Unless that changes with 17 or 18?)


And I'm not sure that renaming the executable is the best approach here.  It technically doesn't satisfy Packaging:Conflicts because upstream isn't doing the renaming.  And the other two choices under Binary_Name_Conflicts are for cases with the same or similar functionality, so they don't apply.

Is there any precedent for installing a conflicting binary someplace other than /usr/bin/?  For my immediate use case, that would be fine because the binary is called by another, which must be informed of the path.  If not an option, any other suggestions?  par dates from 1993 so I'd like to avoid changing its name.

rpmlint output:
par.i386: W: spelling-error Summary(en_US) reformatter -> reformatted, reformatting, reformatory
par.i386: W: spelling-error Summary(en_US) fmt -> ft, mt, fit
par.i386: W: spelling-error %description -l en_US bodiless -> bodices
par.i386: W: invalid-license Par
1 packages and 0 specfiles checked; 0 errors, 4 warnings

Spec URL: http://www.cs.wustl.edu/~levine/par/par-1.52-5.spec
SRPM URL: http://www.cs.wustl.edu/~levine/par/par-1.52-5.fc16.src.rpm

After seeing the discussion about id-utils on the devel mailing list, I think that the par executable should be installed as /usr/lib/par/par.  So, par-1.52-5.spec installs these files:

/usr/lib/par/par
/usr/share/doc/par-1.52/par.doc
/usr/share/doc/par-1.52/releasenotes
/usr/share/man/man1/par.1.gz

The defattr directive can be removed if install explicitly creates the doc dir.


Output of rpmlint -o 'ValidLicenses ("Par")':

par.i386: W: spelling-error Summary(en_US) reformatter -> reformatted, reformatting, reformatory
par.i386: W: spelling-error Summary(en_US) fmt -> ft, mt, fit
par.i386: W: spelling-error %description -l en_US bodiless -> bodices
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

Added a fc17 src rpm, with contents identical to the fc16 src rpm:

Spec URL: http://www.cs.wustl.edu/~levine/par/par-1.52-5.spec
SRPM URL: http://www.cs.wustl.edu/~levine/par/par-1.52-5.fc17.src.rpm

spot, will you have a chance to look at this?

Thanks very much,

David

Note a note that spot isn't CC'd here so there's not much chance he would see your comment.

I personally do not see the point in /usr/lib/par/par.  If people are supposed to run this, why put it outside of the path?  Of course, I also don't see why the par2cmdline utility has claim the executable name.  This one has nearly 20 years of history, and the par2cmdline package only owns /usr/bin/par because of this line in the spec:

ln -sf par2 $RPM_BUILD_ROOT/%{_bindir}/par

So the par executable there isn't even part of upstream; it's just a fiction of the Fedora package.  I would file a bug to have it removed, and then this could go in properly, at least in rawhide.

Thanks.  I had already been in touch with the Fedora maintainer of par2cmdline.  He explained that it supports two versions of a format, and that the symlink allows one executable to support both.  I wasn't aware that upstream doesn't have that symlink.  I'll revisit with the maintainer.

Spec URL: http://www.cs.wustl.edu/~levine/par/par-1.52-6.spec
SRPM URL: http://www.cs.wustl.edu/~levine/par/par-1.52-6.fc17.src.rpm

Great news from the par2cmdline packager:

  The /usr/bin/par symlink has just been removed in Fedora Rawhide in
  package par2cmdline-0.4.tbb.20100203-10.fc18.

Thank you for the suggestion, Jason.

So we now have:
/usr/bin/par
/usr/share/doc/par-1.52/par.doc
/usr/share/doc/par-1.52/releasenotes
/usr/share/man/man1/par.1.gz

$ rpmlint -o 'ValidLicenses ("Par")' par
par.i686: W: spelling-error Summary(en_US) reformatter -> reformatted, reformatting, reformatory
par.i686: W: spelling-error Summary(en_US) fmt -> ft, mt, fit
par.i686: W: spelling-error %description -l en_US bodiless -> bodices
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

OK, looks pretty good.  Indeed, the Par license is acceptable even though rpmlint will complain about it until it gets an update.

Still one rpmlint complaint you missed, though:
  par-debuginfo.x86_64: E: empty-debuginfo-package
and indeed, the debuginfo package is missing sources.  The compiler is called with the proper flags, so at some point the binary is being stripped.  Turns out you're doing that yourself:
  install -s par %{buildroot}/%{_bindir}
You must not do this.  Just remove the '-s' from that line, so that the file with debugging info gets installed and rpmbuild can do the proper debuginfo extraction.

The only other thing I see after a quick review is that the summary is slightly pretentions.  It is generally best to avoid advertising-like statements in package summaries, since they don't add anything that the user might be looking for.  The "but better" doesn't really serve to describe what the package does.

So one thing that must be fixed, and one minor thing.

* source files match upstream.  sha256sum:
  33dcdae905f4b4267b4dc1f3efb032d79705ca8d2122e17efdecfd8162067082
   Par152.tar.gz
  cc2e2c7943b73d9ea893717f669ec7f1665188dd8bedd45aeaca027c30b056bb
   par_1.52-i18n.3.diff.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
? summary is a bit pretentious.
* description is OK.
* dist tag is present.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package (in par.doc)
* latest version is being packaged.
* BuildRequires are proper (none).
* compiler flags are appropriate.
* package builds in mock (rawhide, x86_64).
* package installs properly.
X debuginfo package is empty.
X rpmlint has a valid complaint.
* final provides and requires are sane:
   par = 1.52-6.fc18
   par(x86-64) = 1.52-6.fc18
  =
   (none)

* no bundled libraries.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no static libraries.
* no libtool .la files.

Spec URL: http://www.cs.wustl.edu/~levine/par/par-1.52-7.spec
SRPM URL: http://www.cs.wustl.edu/~levine/par/par-1.52-7.fc17.src.rpm

I had already gone back and forth on the -s.  Without it, there's a warning from rpmlint complaining about not having it :-/

The par summary that says that it's better than fmt has been floating around since 2002, at least.  And its man page justifies the claim.  But I changed it to be something that I think is less pretentious but still helpful.

$ rpm -qf `which rpmlint`
rpmlint-1.4-6.fc17.noarch
$ rpmlint -o 'ValidLicenses ("Par")' par
par.i386: W: spelling-error Summary(en_US) reformatter -> reformatted, reformatting, reformatory
par.i386: W: spelling-error Summary(en_US) fmt -> ft, mt, fit
par.i386: W: spelling-error %description -l en_US bodiless -> bodices
par.i386: W: unstripped-binary-or-object /usr/bin/par
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

I did test taking out the -s and I get no additional rpmlint warnings.  This is building on rawhide x86_64 and i686 and testing the packages with rpmlint 1.4-6.fc17.  But I'm rather confused about how you get an actual i386 build; Fedora hasn't built i386 packages, or used package names with "i386", for quite some time now.  Here's a scratch build: 
  http://koji.fedoraproject.org/koji/taskinfo?taskID=4227580
Note that no "i386" packages generated.  Grabbing the i686 packages and running rpmlint on them complains about nothing save the license tag.

Anyway, the current package looks good.  You have been sponsored in the account system, and once the permissions propagate you will be able to make your SCM request.  Please feel free to meet me on IRC in #fedora-devel if you need guidance through the process of getting your package into the system.

APPROVED

Thank you.

I verified that I don't have any ~/.rpm* files.  I'm running vanilla Fedora 17 on an older Pentium.  uname -a reports it as an i686:
  Linux sna 3.3.4-5.fc17.i686 #1 SMP Mon May 7 17:45:26 UTC 2012 i686 i686 i386 GNU/Linux

But whatever rpmbuild uses to detect the arch is calling it an i386.  Any suggestions on where to look next, or report it?

$ rpmbuild --showrc 2>&1 | head -12
ARCHITECTURE AND OS:
build arch            : i386
compatible build archs: pentium3 i686 i586 i486 i386 noarch fat
build os              : Linux
compatible build os's : Linux
install arch          : pentium3
install os            : Linux
compatible archs      : pentium3 i686 i586 i486 i386 noarch fat
compatible os's       : Linux

RPMRC VALUES:
optflags              : -O2 -g -march=pentium3

Disregard Comment 15.  I installed redhat-rpm-config and now rpmbuild --showrc reports the build arch as an i686.

I do hope you know that you should be doing test builds in mock or in our buildsystem.  Doing local builds does not properly test buildability.

New Package SCM Request
=======================
Package Name: par
Short Description: Paragraph reformatter, vaguely like fmt, but more elaborate
Owners: levined
Branches:
InitialCC:

Git done (by process-git-requests).