Bug 225255 - Merge Review: arptables_jf
Merge Review: arptables_jf
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michal Sekletar
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-29 16:02 EST by Nobody's working on this, feel free to take it
Modified: 2012-06-15 12:19 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-30 06:39:35 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-29 16:02:07 EST
Fedora Merge Review: arptables_jf

http://cvs.fedora.redhat.com/viewcvs/devel/arptables_jf/
Comment 1 manuel wolfshant 2007-03-05 05:18:06 EST
MUSTFIX
- Epoch tag can be removed (0 is the default value)
- BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n
- no URL provided
- "BuildPrereq: /usr/bin/perl" can be removed, perl is on the exception list; if
you plan to keep it then you should use "BuildRequires(pre)" (and eventually
explain why you explicitely want it )
- "Requires(post,postun): chkconfig" should be split in Requires(post),
Requires(postun)
- You should stick with either %{buildroot} or $RPM_BUILD_ROOT but not both
- "rm -rf %{buildroot}" is not needed in %prep
- "rm -rf $RPM_BUILD_ROOT" is missing in %install
- "service" should be added to Requires
- Missing SMP flags. If it doesn't build with it, please add a comment
- $RPM_OPT_FLAGS is not used
- "%config /etc/rc.d/init.d/arptables_jf" should use a macro, not a fixed path

SHOULD FIX
- Summary ended with dot
- adding "INSTALL="%{__install} -c -p" to the make install line would preserve
timestamps


rpmlint has some info for us, too:
rpmlint of arptables_jf:
W: arptables_jf summary-ended-with-dot Userspace control program for the
arptables network filter.
W: arptables_jf no-url-tag
- We know about these two already

W: arptables_jf conffile-without-noreplace-flag /etc/rc.d/init.d/arptables_jf
E: arptables_jf executable-marked-as-config-file /etc/rc.d/init.d/arptables_jf
- /etc/rc.d/init.d/arptables_jf should not be marked as %config; could be left
as such till F8T1 according to last week's guidelines

W: arptables_jf service-default-enabled /etc/rc.d/init.d/arptables_jf
- that's a good point. Do we need/want it enabled at start time?

E: arptables_jf incoherent-subsys /etc/rc.d/init.d/arptables_jf arptables
E: arptables_jf incoherent-subsys /etc/rc.d/init.d/arptables_jf arptables
E: arptables_jf incoherent-subsys /etc/rc.d/init.d/arptables_jf arptables
- Harmless, but maybe should be discussed. Why is the package called
arptables_jf and not arptables ?

W: arptables_jf no-reload-entry /etc/rc.d/init.d/arptables_jf
- harmless, although a reload which does just start+stop would be easy to implement
Comment 2 manuel wolfshant 2007-03-05 05:22:02 EST
wrt "BuildRequires(pre)" .. should be read "BuildRequires"
Comment 3 Gabriel Somlo 2008-08-13 13:54:58 EDT
any idea what the difference between this package's source tree
and arptables-0.0.3-3 from ebtables.sourceforge.net ? Is this package a
fork ? Any pointers to the story behind how we ended up with two different
ones would be much appreciated...
Comment 4 Jiri Skala 2010-01-22 09:40:53 EST
(In reply to comment #3)
> any idea what the difference between this package's source tree
> and arptables-0.0.3-3 from ebtables.sourceforge.net ? Is this package a
> fork ? Any pointers to the story behind how we ended up with two different
> ones would be much appreciated...    

No idea what should be marked as a fork. These two packages have the same basis.
Comment 5 Jiri Skala 2010-01-22 09:43:11 EST
Some troubles complained by rpmlint are fixed.
Comment 6 Jiri Skala 2010-01-28 04:59:29 EST
arptables_jf.x86_64: W: no-url-tag
COMMENT: upstream dead

arptables_jf.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/arptables_jf
COMMENT: intention

arptables_jf.x86_64: E: incoherent-subsys /etc/rc.d/init.d/arptables_jf arptables
arptables_jf.x86_64: E: incoherent-subsys /etc/rc.d/init.d/arptables_jf arptables
arptables_jf.x86_64: E: incoherent-subsys /etc/rc.d/init.d/arptables_jf arptables
COMMENT: wan't fix. This makes sense when new package is inserted to Fedora. Trying change it now could generate difficulties to users.
Comment 7 manuel wolfshant 2010-08-20 07:41:03 EDT
rechecking Version: 0.0.8 / Release: 19%{?dist}

Please add a comment in the spec file, explaining that the upstream is dead and that the following two warnings cannot be avoided:
  arptables_jf.src: W: no-url-tag
  arptables_jf.src: W: invalid-url Source0: arptables_jf-0.0.8.tbz



As of the incoherent-subsys error, I do not agree. I doubt that a sysadmin which requires this package will be puzzled by the fact that the lock file is called arptables or arptables_jf.
Comment 8 Jan Engelhardt 2011-01-01 14:15:07 EST
(In reply to comment #4)
> (In reply to comment #3)
> > any idea what the difference between this package's source tree
> > and arptables-0.0.3-3 from ebtables.sourceforge.net ? Is this package a
> > fork ? Any pointers to the story behind how we ended up with two different
> > ones would be much appreciated...    
> 
> No idea what should be marked as a fork. These two packages have the same
> basis.

Looks pretty much like arptables_jf is the fork, since it reuses the arptables name with a _jf suffix, was created by a Redhat employee, and is also only used by Fedora.
Comment 9 Michal Sekletar 2011-08-16 03:09:34 EDT
Checked git commit
8b1d1dbab8f89c2301349f176aa57cd049cbbdf5 
NO  source files match upstream 
  - there is no upstream for arptables_jf package
YES package meets naming and versioning guidelines
  - package name contains underscore but package arptables_jf is on the
    list of packages which don't have to obey this particular guideline
    https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Separators
YES spec file is properly named, is cleanly written and uses macros consistently
YES dist tag is present
YES clean section and buildroot present
YES licence field matches the actual license
NO  license text included in package
  - no text file with license is not included in source tarball, it should be 
    present
YES latest version is being packaged
NO  BuildRequiers are proper (Perl?)
  - I think perl is not needed to properly build the package
YES compiler flags are appropriate
YES package builds in mock
YES debuginfo package looks complete
NO  rpmlint is silent
  - $ rpmlint arptables_jf.spec
    arptables_jf.spec: W: invalid-url Source0: arptables_jf-0.0.8.tbz
    0 packages and 1 specfiles checked; 0 errors, 1 warnings.
    
    - warning about invalid-url is ok since there's no upstream, thus there's
      no url pointing to upstream's remote source code tarball

  - $ rpmlint arptables_jf-0.0.8-22.fc17.src.rpm
    arptables_jf.src: W: spelling-error Summary(en_US) Userspace -> User space, User-space, Users pace
    arptables_jf.src: W: spelling-error Summary(en_US) arptables -> portables, stables, tables
    arptables_jf.src: W: spelling-error %description -l en_US arptables -> portables, stables, tables
    arptables_jf.src: W: spelling-error %description -l en_US jf -> hf, jg, j
    arptables_jf.src: W: spelling-error %description -l en_US arpfilter -> filterer
    arptables_jf.src: W: spelling-error %description -l en_US firewalling -> fire walling, fire-walling, firewall
    arptables_jf.src: W: spelling-error %description -l en_US arp -> rap, tarp, carp
    arptables_jf.src: W: no-url-tag
    arptables_jf.src: W: invalid-url Source0: arptables_jf-0.0.8.tbz
    1 packages and 0 specfiles checked; 0 errors, 9 warnings.
    
    - spelling errors could be safely ignored
    - no url tag and invalid source warnings could be ignored because there
      is no upstream

  - $ rpmlint arptables_jf-0.0.8-22.fc17.x86_64.rpm
    arptables_jf.x86_64: W: spelling-error Summary(en_US) Userspace -> User space, User-space, Users pace
    arptables_jf.x86_64: W: spelling-error %description -l en_US jf -> hf, jg, j
    arptables_jf.x86_64: W: spelling-error %description -l en_US arpfilter -> filterer
    arptables_jf.x86_64: W: spelling-error %description -l en_US firewalling -> fire walling, fire-walling, firewall
    arptables_jf.x86_64: W: spelling-error %description -l en_US arp -> rap, tarp, carp
    arptables_jf.x86_64: W: incoherent-version-in-changelog 0:0.0.8-22 ['0.0.8-22.fc17', '0.0.8-22']
    arptables_jf.x86_64: W: no-url-tag
    arptables_jf.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/arptables_jf
    arptables_jf.x86_64: E: incoherent-subsys /etc/rc.d/init.d/arptables_jf arptables
    arptables_jf.x86_64: E: incoherent-subsys /etc/rc.d/init.d/arptables_jf arptables
    arptables_jf.x86_64: E: incoherent-subsys /etc/rc.d/init.d/arptables_jf arptables
    1 packages and 0 specfiles checked; 3 errors, 8 warnings.

    - spelling errors could be ignored
    - incoherent version in changelog warning is caused by use of dist tag and 
      could be safely ignored
    - incoherent-subsys, changes due to satisfying rpmlint could break user's 
      scripts, thus warning is ignored
    - spelling errors could be ignored

  - $ rpmlint arptables_jf-debuginfo-0.0.8-22.fc17.x86_64.rpm
    arptables_jf-debuginfo.x86_64: W: spelling-error Summary(en_US) arptables -> portables, stables, tables
    arptables_jf-debuginfo.x86_64: W: spelling-error Summary(en_US) jf -> hf, jg, j
    arptables_jf-debuginfo.x86_64: W: spelling-error %description -l en_US arptables -> portables, stables, tables
    arptables_jf-debuginfo.x86_64: W: spelling-error %description -l en_US jf -> hf, jg, j
    arptables_jf-debuginfo.x86_64: W: no-url-tag
    1 packages and 0 specfiles checked; 0 errors, 5 warnings.

    - spelling errors could be ignored
    - no-url-tag warning is ok because there's no upstream

YES final provides and requires look sane
N/A %check is present and all tests pass
YES no shared libraries are added to the regular linker search paths
N/A owns the directories it creates
  - package content is copied into already existing directories
YES doesn't own any directories it shouldn't
YES no duplicates in %files
YES scriptlets looks sane
YES code, not content
N/A large documentation files must go in a -doc subpackage.
YES %docs are not necessary for the proper functioning of the package.
YES no headers.
YES no pkgconfig files.
YES no libtool .la droppings.
YES not a GUI app.
Comment 10 Jon Ciesla 2012-04-05 09:25:02 EDT
Ping?
Comment 11 Jon Ciesla 2012-04-16 15:37:01 EDT
Adding owner.
Comment 12 Jiri Popelka 2012-05-16 13:50:14 EDT
(In reply to comment #9)
> Checked git commit
> 8b1d1dbab8f89c2301349f176aa57cd049cbbdf5 
> NO  source files match upstream 
>   - there is no upstream for arptables_jf package

I put a comment to the Source0 tag.

> NO  license text included in package
>   - no text file with license is not included in source tarball, it should be 
>     present

Licensing guidelines says that:
"If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc. If the source package does not include the text of the license(s), the packager should contact upstream and encourage them to correct this mistake."

The license text is not included in the tarball and upstream is dead so there's
nothing I can do here.

> NO  BuildRequiers are proper (Perl?)
>   - I think perl is not needed to properly build the package

Fixed in rawhide.
Comment 13 Jiri Popelka 2012-05-30 06:17:09 EDT
I don't see anything else to fix. Can we close this review ?
Comment 14 Michal Sekletar 2012-05-30 06:39:35 EDT
I think we can close this one.
Comment 15 Parag AN(पराग) 2012-06-15 12:19:45 EDT
This bug just popped out when I searched for packages under review but this looks completed without setting fedora-review+

Setting the flag therefore.

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