* There are icons directly in %{_datadir}/icons/ they certainly should 
  be below hicolor.

* A .desktop file is missing for xt5250

* -devel should

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

* missing devel dependencies, on openssl-devel and ncurses-devel and
  pkgconfig

* /usr/share/aclocal/ is unowned in devel

* some dependencies are missing for the pre/post/... scripts.

* BuildRoot is wrong

* Prefix: /usr is certainly wrong too.

* Isn't the Requires ncurse automatically handled?

* Is it really necessary to rerun the autotools?

* %{?_smp_mflags} is missing for make

* Requires: xterm missing for xt5250

* rpmlint will still say
W: tn5250 macro-in-%changelog post
W: tn5250 mixed-use-of-spaces-and-tabs (spaces: line 70, tab: line 1)
W: tn5250-devel summary-not-capitalized development tools for the 5250 protocol.
W: tn5250-devel summary-ended-with-dot development tools for the 5250 protocol.

Suggestions:

* The %files section is certainly over-complicated.

* I would personally have removed the -f from rm, such that it fails
  whenever the target file doesn't exist anymore.

* It is better to add a -p to install calls to keep timestamps, and
  also providing -m xxx for the permissions is relevant.

* add a -b to %patches such that it is easy for other contributors
  to modify and rerun gendiff.

* use %_dist in release

* remove the static libs, adding --disable-static to %configure
  should certainly be enough.

all except suggestions 2 and 4 fixed in tn5250-0.17.3-8.fc7
- failed scripts often lead to duplicate packages in the rpmdb, I'd like to
avoid that
- %patch -b can lead to rpms containing backup files if you aren't careful. It
happened in the past and will happen again. 
I'd suggest the opposite: remove all -b flags and add them only when you need
them  for gendiff.

(In reply to comment #2)
> all except suggestions 2 and 4 fixed in tn5250-0.17.3-8.fc7
> - failed scripts often lead to duplicate packages in the rpmdb, I'd like to
> avoid that

You are right, and I agree with you. I didn't targeted the rm -f in 
scriptlets, but in %install:
rm -f $RPM_BUILD_ROOT/%{_libdir}/lib5250.la

> - %patch -b can lead to rpms containing backup files if you aren't careful. It
> happened in the past and will happen again. 
> I'd suggest the opposite: remove all -b flags and add them only when you need
> them  for gendiff.

Ok, as you like.



I haven't seen the changes, so I guess it takes time for changes in 
internal cvs to propagate.

There is still a missing
Requires(post): /sbin/ldconfig

The Application; and X-Red-Hat-Base categories shouldn't be added, 
in my opinion. And the Network category would better be in the
.desktop file instead of added in the spec file.

In the xt5250.desktop file there shouldn't be any Mimetype entry.

If I recall well the guidelines, --vendor should be fedora.

The autotools are rerun during the build, certainly because the patching
of autotool files makes some files newer that generated files.
You can fix that by touching generated files, or by keeping the 
original timestamps when patching (that can be achieved with cp -p
and touch -r).

I think it would be better if tn5250-48x48.{png,xpm} were called 
tn5250.{png,xpm} in /usr/share/icons/hicolor/48x48/apps/, and
tn5250-62x48.{png,xpm} were put in /usr/share/icons/hicolor/64x64/apps and
also called tn5250.{png,xpm}.

Changes in -10
- rename icon files to tn5250.{png,xpm}
- remove Mimetype from desktop file
- move category to desktop file
- use vendor fedora for desktop-file-install
- touch files to avoid autotools run

* change devel defattr, current is broken. I suggest using everywhere
%defattr(-, root, root, -)

* icons should be installed with -m644, like
install -p -m644 tn5250-48x48.png
$RPM_BUILD_ROOT/%{_datadir}/icons/hicolor/48x48/apps/tn5250.png

Same for tcap and terminfo files

* You shouldn't touch all the files, only the the ones that had
  their timestamps messed up, I guess configure, configure.in 
  and Makefile.in

I usually prefer setting all permissions in the %files section as you can't end
up with some unwantend world-writable files this way. But I agree that this can
only happen when you rely on 'make install' and not when you run the install
binary on your own.
Anyway, fixed in -11

The touch is still a bit wrong, since aclocal.m4 is more
recent. Moreover the timestamp of Makefile.in needs not to be. 
The following seems better:

touch -r aclocal.m4 configure configure.in

Maybe you could use %defattr(-,root,root,-) also for the main package?

INSTALL in %doc is unusefull.

The remaining rpmlint output seems harmless to me:
W: tn5250 dangerous-command-in-%preun rm
W: tn5250-devel no-documentation

fixed in tn5250-0.17.3-12.fc7

Issues:

* libtool BuildRequires is certainly unneeded

* a post scriptlet for the hicolor icon theme is missing

* --short-circuit will certainly fail with 
mv linux/README README.Linux

Suggestion:

* remove / in  $RPM_BUILD_ROOT/%{_datadir}, $RPM_BUILD_ROOT/%{_libdir}

* I would do something like 
mv linux/README README.Linux
in %install 

I prefer having slashes between the macros for better readability. It shouldn't
do any harm as _libdir and _datadir never have relative paths.

The rest is fixed in -13

* Still many warnings from desktop-file-install (not blocking)

/home/dumas/src/fc-cvs/tn5250/devel/xt5250.desktop: missing encoding  (guessed
UTF-8)
/home/dumas/src/fc-cvs/tn5250/devel/xt5250.desktop: key "Categories" string list
not semicolon-terminated, fixing
/var/tmp/tn5250-0.17.3-13-root-dumas//usr/share/applications/fedora-xt5250.desktop:
warning: boolean key "Terminal" has value "0", boolean values should be "false"
or "true", although "0" and "1" are allowed in this field for backwards
compatibility
/var/tmp/tn5250-0.17.3-13-root-dumas//usr/share/applications/fedora-xt5250.desktop:
warning: non-standard key "XClassHintResName" lacks the "X-" prefix
/var/tmp/tn5250-0.17.3-13-root-dumas//usr/share/applications/fedora-xt5250.desktop:
warning: file contains key "MapNotify", usage of this key is not recommended,
since it has been deprecated


* after rpmbuild -ba, rpmbuild -bi --short-circuit fails with:
+ mv linux/README README.Linux
mv: cannot stat `linux/README': No such file or directory

Instead of mv you could do

cp -pf linux/README README.Linux

Also your icon cache scriptlets may be right, but to be on the
safe side, and for consistency, I suggest using those from the
guidelines, at
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

I'm glad that you know all those wiki pages, I can't seem to find them when I
look for them ;-(
fixed in tn5250-0.17.3-14.fc7

I saw them growing...

The source file timestamp is not the same than upstream, remember
to keep it in the future, for example with spectool -g, wget -N.

Otherwise source match upstream, and I see no issue. There is 
%{_datadir}/icons/hicolor/*/apps/ which may be unowned, but
this should be fixed once for all not here.

APPROVED

Closing as suggested by Warren in his mail to fedora-maintainers 'Review with
Flags (Version 6)'