[Bug 446102] Review Request: xdialog - X11 drop in replacement for cdialog

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: xdialog -  X11 drop in replacement for cdialog


https://bugzilla.redhat.com/show_bug.cgi?id=446102





------- Additional Comments From pertusus at free.fr  2008-06-25 14:42 EST -------
(In reply to comment #2)
> -Release: 1%{dist}
> +Release: 1%{?dist}
> * The preferred dist tag is now ?dist.

Thanks. Not only is it preferred, but it is bad to have %{dist} since it
may not be defined.

> -License: GPL+
> +License: GPLv2
> * License should be as concrete as possible, in source archive is GPLv2

I don't understand. All the files in src seems not to have any license
header, which means GPL+. Am I missing something?

> -URL: http://xdialog.dyns.net/
> +URL: http://xdialog.free.fr

Indeed, looks like it is better to use xdialog.free.fr.

> -BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root
> +BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
> 
> * Tip: this is #1 in "BuildRoot tag" section of
> https://fedoraproject.org/wiki/Packaging/Guidelines

I prefer a reproducible buildroot. I changed it to the 2nd BuildRoot
tag, though.

>  BuildRequires: gtk+-devel >= 1.2.0
> 
> * Isn't it possible to completely get rid of GTK+v1? It's ugly, not widely
> supported these days and adds build dependency.

For this package it could have been the converse. As said above in 
Comment #1 the author prefers the gtk1 version. Besides build 
dependencies are not an issue. It is true that gtk1 is lacking some
features, prominently utf8, still it is not a reason to leave it
apart when upstream advertises to use it.

> -%{__rm} -rf %{buildroot}
> +rm -rf %{buildroot}
> 
> * Be consistent, use command style OR macro style.
>  
> -%defattr(-, root, root, 0755)
> +%defattr(-, root, root, -)

Both issues are not a big deal in my opinion, but changed anyway.

> -%doc AUTHORS BUGS ChangeLog COPYING NEWS README
> +%doc AUTHORS BUGS ChangeLog COPYING 
> 
> * IMO, useless for docs.
> * README is not maintained for years (just read it) and NEWS is symlink to
> ChangeLog.

I'll leave the README, it has meaningful informations in it and the 
fact that it is no more maintained is plainly documented.

> -%{_mandir}/man1/Xdialog.1*
> +%{_mandir}/man?/%{real_name}*
> 
> * This is more general way how to play with man pages, don't have to care of
> every one page and of the section. 

I prefer listing files more precisely such that build fails if file
name changes or new file appear.
 
> -* Sat Apr  5 2008 Patrice Dumas <pertusus at free.fr> 2.3.1-1
> -- submit to fedora.
> +* Sat Apr  5 2008 Patrice Dumas <pertusus at free.fr> - 2.3.1-1
> +- Submit to Fedora.
>  
> * Just some more consistency issues.

I prefer keeping the changelog of Dag/Dries like they want it to be
formatted, but switch to my preferred format in fedora, so I'll 
leave it as is. 

> Please see the output of rpmlint on arch dependent package (e.g. i386) you'll
> see lot of warning about +x on doc files:
> 
>    xdialog.i386: W: spurious-executable-perm
> /usr/share/doc/xdialog-2.3.1/samples/timebox
> 
> * Change it to 0644 or erase them.

Nope, they are sample that can be run as is and are right to be there 
and executable, rpmlint cannot always be right.

Thanks for the review. Are you waiting to be sponsored?

Updated package:
http://www.environnement.ens.fr/perso/dumas/fc-srpms/xdialog-2.3.1-2.fc10.src.rpm

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.