This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 226546 - Merge Review: wvdial
Merge Review: wvdial
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michal Hlavinka
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:17 EST by Nobody's working on this, feel free to take it
Modified: 2009-11-30 05:53 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-11-30 05:08:03 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
mhlavink: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:17:27 EST
Fedora Merge Review: wvdial

http://cvs.fedora.redhat.com/viewcvs/devel/wvdial/
Initial Owner: harald@redhat.com
Comment 1 Richard MacKenzie 2009-09-05 12:44:10 EDT
wvdial 1.60-8 will not install on Fedora 11
Comment 2 Michal Hlavinka 2009-11-27 10:37:59 EST
0) MUST:
* MUST: rpmlint :

$ rpmlint wvdial.spec wvdial-*.src.rpm x86_64/wvdial-*
wvdial.x86_64: E: zero-length /etc/wvdial.conf
wvdial.x86_64: E: non-readable /etc/ppp/peers/wvdial 0600
3 packages and 1 specfiles checked; 2 errors, 0 warnings.

looks ok


Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+ MUST: The package must be named according to the Package Naming Guidelines
+ MUST: The spec file name must match the base package %{name}, in the format %{name}.spec
+ MUST: The package must meet the Packaging Guidelines
+ MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
- MUST(4): The License field in spec match the actual license
+ MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file must be included in %doc
+ MUST: The spec file written in American English
+ MUST: The spec file for the package is legible
- MUST(6): The sources used to build the package must match the upstream source, as provided in the spec URL
+ MUST: The package successfully compile
+ MUST: All build dependencies must be listed in BuildRequires
+ MUST: The spec file handle locales properly
0 MUST: Every package which stores shared library files in any of the dynamic linker's default paths, must call ldconfig in %post and %postun
+ MUST: Packages does not bundle copies of system libraries
+ MUST: Package own all directories that it creates
+ MUST: Package does not list a file more than once in the spec file
+ MUST(2): Permissions on files must be set properly. Every %files section must include a %defattr(...) line
+ MUST(1): Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
+ MUST: Package use macros consistently
+ MUST: Package contains code, or permissable content
+ MUST: Large documentation files must go in a -doc subpackage
+ MUST: If a package includes something as %doc, it must not affect the runtime of the application
0 MUST: Header files in a -devel package
0 MUST: Static libraries in a -static package
0 MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
0 MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package
0 MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
+ MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built
0 MUST: Packages containing GUI applications must include a %{name}.desktop file
+ MUST: Packages must not own files or directories already owned by other packages
+ MUST(1): At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
+ MUST: All filenames in rpm packages must be valid UTF-8. [27]


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

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

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.



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."""

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

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.

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" ?

please fix these issues, thanks
Comment 3 Ondrej Vasik 2009-11-27 11:54:25 EST
(In reply to comment #2)
> - MUST(4): The License field in spec match the actual license

Ok, used LGPLv2+ instead of LGPLv2

> - MUST(6): The sources used to build the package must match the upstream
> source, as provided in the spec URL

Must be some kind of alumnit temporary shutdown, I downloaded new version of wvdial (1.61) from there ~2 weeks ago...

> 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...

> 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...

> 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.

> 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+

> 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...

> 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...

> 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. 

Fixed in rawhide ... wvdial-1.61-2.fc13
Comment 4 Michal Hlavinka 2009-11-30 04:17:26 EST
(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
Comment 5 Ondrej Vasik 2009-11-30 04:51:26 EST
1) and 6) fixed in wvdial-1.61-3.fc13.
Comment 6 Michal Hlavinka 2009-11-30 05:08:03 EST
(In reply to comment #5)
> 1) and 6) fixed in wvdial-1.61-3.fc13.  

verified, no other objections

thanks
Comment 7 Ondrej Vasik 2009-11-30 05:53:33 EST
Thanks for review...

Note You need to log in before you can comment on or make changes to this bug.