Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226546 --- Comment #4 from Michal Hlavinka <[email protected]> 2009-11-30 04:17:26 EDT --- (In reply to comment #3) > (In reply to comment #2) > > Comments: > > > > 1) Checking RPM_BUILD_ROOT != / is not necessary > > > > per Packaging Guidelines ( > > https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean ): > > > In the past, some packages checked that %{buildroot} was not / before > > > deleting it. This is not necessary in Fedora, .... > > > > rm -rf $RPM_BUILD_ROOT is enough > > Ok, changed... improvement There's no need to check RPM_BUILD_ROOT != / in other sections too. It can be removed from %install section. This section is executed during build, not during package install, so it's not going to eat / neither. > > 2) %attr in %files section is used too much > > > > %attr(0755,root,root) %{_bindir}/* > > %attr(0644,root,root) %{_mandir}/man1/* > > %attr(0644,root,root) %{_mandir}/man5/* > > > > these are default permissions, thus not required to explicitly add there > > Ok, removed... verified > > 3) too much wildcards under %files section > > > > If upstream makes some changes in tarball and add/remove some files, this is > > not going to catch anything. It's good practice to list at least all files > > under %{_bindir}. This will let you know if there is any new/missing one. > > files under %{_bindir} and man pages listed more specifically. verified > > > 4) License > > > > There is no license info in the package except COPYING - LGPL. This means > > License tag should be set to LGPLv2+ > > > > http://fedoraproject.org/wiki/Licensing : > > > > """A GPL or LGPL licensed package that lacks any statement of what version > > that > > it's licensed under in the source code/program output/accompanying docs is > > technically licensed under *any* version of the GPL or LGPL, not just the > > version in whatever COPYING file they include. Note that this is LGPLv2+, > > not > > LGPL+, because version 2 was the first version of LGPL.""" > > Ok, changed LGPLv2 to LGPLv2+ verified > > 5) Versioned requires ( > > https://fedoraproject.org/wiki/Packaging:Guidelines#Requires ) > > > > > First, if the lowest possible requirement is so old that nobody has a > > > version older than that installed on any target distribution release, > > > there's no need to include the version in the dependency at all. In that > > > case we know the available software is new enough. For example, the > > > version in gtk+-devel 1.2 dependency above is unnecessary for all Red Hat > > > Linux distributions since (at least) release 6.2. As a rule of thumb, if > > > the version is not required, don't add it just for fun. > > > > all 'ppp' versions (even in old RHELs) are newer than version specified, > > please > > remove it > > Removed versioned requires... verified > > > 6)Url and Source0 links does not work > > > > wget http://alumnit.ca/download/wvdial-1.61.tar.gz > > --2009-11-27 16:16:56-- http://alumnit.ca/download/wvdial-1.61.tar.gz > > Resolving alumnit.ca... 69.196.152.118 > > Connecting to alumnit.ca|69.196.152.118|:80... failed: Connection refused. > > > > > > wget 'http://alumnit.ca/wiki/?WvDial' > > --2009-11-27 16:17:30-- http://alumnit.ca/wiki/?WvDial > > Resolving alumnit.ca... 69.196.152.118 > > Connecting to alumnit.ca|69.196.152.118|:80... failed: Connection refused. > > I guess it is temporary issue... we'll see on Monday... Url link works, but Source0 link does not (http error 404). It seems sources has been moved to http://wvstreams.googlecode.com/files/wvdial-1.61.tar.gz $ curl -s http://wvstreams.googlecode.com/files/wvdial-1.61.tar.gz | md5sum acd3b2050c9b65fff2aecda6576ee7bc - $ cat sources acd3b2050c9b65fff2aecda6576ee7bc wvdial-1.61.tar.gz verified: sources matches latest upstream release, but Source0 link needs to be fixed > > 7) Missing info for patches > > > > https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment > > > > Every patch in spec file should contain a comment describing: > > * why is that patch used - bug number is enough > > * upstream information - was it sent upstream (and when)? taken from > > upstream? > > was it accepted/rejected? is this patch "fedora specific" ? > > I added the informations why the patch is used with bug numbers/short > comments. > Some patches - like remotename and 9nums are Fedora specific. Compuserve patch > is just change to use more new Compuserve style (which increases the chance of > succesful connection). That one wvdial.conf manpage patch - I don't know, I'll > try to submit it once the website will be up. Anyway the package is not really > "alive" - current update was just to fix issues with new gcc/glibc. verified, but try to send them upstream please fix 1) and 6) and we're done here -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA contact for the bug. _______________________________________________ Fedora-package-review mailing list [email protected] http://www.redhat.com/mailman/listinfo/fedora-package-review
