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@xxxxxxx 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@xxxxxxx> 2.3.1-1 > -- submit to fedora. > +* Sat Apr 5 2008 Patrice Dumas <pertusus@xxxxxxx> - 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review