New PuTTY 0.54-0.3 rpms for review.

[Dag Wieers]

On Fri, 20 Feb 2004, Gavin Henry wrote:

> Dag Wieers if you have a second, could you look these over? Or anyone else :-)
> 
> http://www.suretecsystems.com/putty/putty.spec 

Obviously what I would do is already in my SPEC file. Let me tell you why 
I do some things differently.


First of all you seem to want to format everything the way you do. And it 
looks pretty nice, but I don't think it is very useful. Aligning ':' is a 
nobel thing but apart from being very time consuming it doesn't gain you 
anything. And I even think it is more difficult to read. (Does vi even 
color-highlight this correctly ??)

Formatting however is mostly a personal preference anyway.


Then putting a line between sections is something that fedora.us thought 
was a very good idea (no arguing about it back then!) and after a few 
months abandoned unanimously...


Adding a BuildRequirement for desktop-file-utils makes sense if you're 
limiting yourself to distributions that have it. RHEL2.1 doesn't and is 
supported until 2006 IIRC. Perl for me belongs to the standard build 
environment and therefor doesn't need mentioning.


You seem to 'quiet' the %setup output, which is something I _want_ to have 
in my buildlogs. If people are building my source-package I want them to 
be able to even compare the content of a tarball. It also helps to 
identify extra documentation or other files after inspection a buildlog 
when you go over your buildlog one last time. (part of the methodology, in 
the end this saves a lot of time as you don't have to re-release a package 
because you omitted something by mistake)


You've put my perl one-liner on one-line ;)) But you're doing 3 
substitutions in one go and it becomes less obvious what you're doing if 
you make it unreadable this way.

eg.
	%{__perl} -pi -e ' s,-O2,$RPM_OPT_FLAGS,g;s,/usr/local,%{_prefix},g;s,^(\t\$\(INSTALL_DATA\) -m 644 ps.+)$, ,' unix/Makefile.gtk

vs.
	%{__perl} -pi.orig -e '
	                s|-O2|%{optflags}|g;
	                s|/usr/local|%{_prefix}|g;
	                s|^(\t\$\(INSTALL_DATA\) -m 644 ps.+)$|#$1|;
	        ' unix/Makefile.gtk

Simplicity comes first, readability second. Putting a perl one-liner on a 
single line you're not making this more simple, but sure less readable. 
(Remember that when it gets fixed upstream you should verify that it can 
be removed, if it takes you 1 minute to understand what it did...)


Some of the stuff was already mentioned. You're not using 
%{_datadir}/applications, you're adding yet another vendor.


I would add the desktop-file inline so that it is one less extra source 
which greatly enhances your ability to have a clear overview of 
everything. In your case to evaluate the package you need to look at at 
least another file.


I also noticed you're repeating the package-name in your description, eg.

	Summary: PuTTY - A Free Telnet/SSH Client, plus utilities

There's no added value by doing so. What's even more people don't know 
exactly what to expect. If you have alternative software packaged, the 
summary should indicate what the difference is.

Look at:

	openssh - The OpenSSH implementation of SSH protocol versions 1 and 2.
	telnet - The client program for the telnet remote login protocol.
	putty - GUI SSH, Telnet and Rlogin client.
	rsh - Clients for remote access (rsh, rlogin, rcp).

At least you can tell from the summary putty is a GUI program, which is 
essential information for someone who needs to choose. I also try to avoid 
using GNOME/GTK/KDE directly as it shouldn't matter to the end-user.

I also end a summary with a dot, which is often done by Red Hat too. Once 
again it may be a personal preference.


Also you're using $RPM_BUILD_ROOT instead of %{buildroot} and although 
there's no real difference, you're mostly using macros everywhere else. 
This has been discussed at fedora.us also and I'm not sure if they have a 
valid reason to use a shell variable instead of the macro. (I know jbj 
made a statement about it and I don't think it adds to the equation)

To me (personally) the macro fits better and makes it easier for the eyes. 
(FOR THE SAME PEOPLE THINK THAT I'M NOW SHOUTING AND IT HURTS THE EYES ;-)


Well, those are my suggestions and some go directly against fedora.us 
guidelines. So be it.

--   dag wieers,  dag at wieers.com,  http://dag.wieers.com/   --
[Any errors in spelling, tact or fact are transmission errors]