Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 451189 (rancid) - Review Request: rancid - Really Awesome New Cisco confIg Differ
Summary: Review Request: rancid - Really Awesome New Cisco confIg Differ
Keywords:
Status: CLOSED DUPLICATE of bug 512068
Alias: rancid
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 455713
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-06-13 10:48 UTC by Åge Olai Johnsen
Modified: 2009-07-16 07:16 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-01-25 07:37:27 UTC


Attachments (Terms of Use)
patch to honor cflags correctly (deleted)
2008-09-26 18:49 UTC, Mamoru TASAKA
no flags Details

Description Åge Olai Johnsen 2008-06-13 10:48:28 UTC
Spec URL: http://files.thaumaturge.org/users/dante/rancid/rancid.spec
SRPM URL: http://files.thaumaturge.org/users/dante/rancid/rancid-2.3.2a8-0.1.src.rpm
Description: RANCID monitors a router's (or more generally a device's) configuration, including software and hardware (cards, serial numbers, etc) and uses CVS (Concurrent Version System) or Subversion to maintain history of changes.

This is my first package and I'm looking for a sponsor.

I'm not quite sure how this will work with the licensing. They are in the file "COPYING" and URL http://www.shrubbery.net/rancid/LICENSE.txt. For some reason this is not the same license. "COPYING" came with the source and is newer but the developers website state that the URL is the license for Rancid.

Comment 1 Mamoru TASAKA 2008-06-29 18:26:57 UTC
Some ramdom comments:
* Licensing
  - Well, the license of LICENSE.txt as shown on the URL is actually NON-FREE, so
    the license cannot be accepted on Fedora. However as far as I checked the source
    tarball, this software is licensed under "BSD with advertising".
    Note that anyway we don't allow the license tag such as
    "Freely redistributable without restriction".

* Versioning
  - This package seems to be a pre-version for 2.3.2. In such case the current
versioning
    is wrong. Please refer to:
    http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages

* builds
  - Your package does not build:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=685315
    build.log says that at least "ping" binary (in iputils) is missing from
BuildRequires.

* perl module dependency
  - When adding perl module dependency as (Build)Requires, please don't write
the rpm
    names directly but write what modules the rpms provide:
    https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides

* Macros
  - Use macros correctly. For example, /etc must be %{_sysconfdir}.
  - Also, using the directory %{_prefix}/local is not allowed on Fedora.

* Inproper scriptlets
  - You must call "mkdir" "chown" "ln" "chmod" commands on scriptlets except for
very special
    cases.
    As same as other packages, for this package you must create the needed
directories
    by the time %install ends, and must add those directories to %files entry and
    set attribute correctly by using %attr.

* Directory ownership issue
  - Please make it sure that all directories which are created by installing
this packages
    are correctly owned by this package.
    Currently %{_sysconfdir}/rancid/, %{_datadir}/rancid/ are not owned by any
packages.

* %changelog version
  - The EVR (Epoch:Version:Release) number of this package does not coincide
with the
    last entry on %changelog.

Please make it sure that when you modify your spec file, you also change the
release number
of the spec file to avoid confusion.


Comment 2 Åge Olai Johnsen 2008-07-09 10:38:08 UTC
Thank you for your feedback.

I've updated the spec-file with fixes to most of the comments, added a patch 
for the config-file and a cronjob.
I'm still waiting for a email from the developer about the license, until then 
i'm using the "BSD with advertising" from the tarball.

Still puzzled by the versioning.. Version 2.3.2a8 from the developer; would it 
then be rancid-2.3.2a8-0.2, rancid-2.3.2-0.2.a8 or rancid-2.3.2a8-0.2.a8..? 
(%{version} something else then developerversion seems to complicate the build, 
and I have not found similar examples to compare solutions with)

New specfile and SRPM:
http://files.thaumaturge.org/users/dante/rancid/rancid-0.2.spec
http://files.thaumaturge.org/users/dante/rancid/
rancid-2.3.2a8-0.2.a8.fc9.src.rpm


Comment 3 Mamoru TASAKA 2008-07-13 15:45:09 UTC
Well:

* Versioning
  - As the tarball included in the tarball seems pre-version, the versioning
    should be: 2.3.2-0.XXX.a8%{?dist} where XXX should be incremented every
    time you modify your spec file.

* optflags
  - Fedora specific compilation flags are not passed correctly to C compiler.
    https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
    You can check what compiler flags are used by
    $ rpm --eval %optflags
    Also please check the logs printeded out by rpmbuild command.
--------------------------------------------------------------
   365  Making all in bin
   366  gmake[1]: Entering directory `/builddir/build/BUILD/rancid-2.3.2a8/bin'
   367  gcc -DHAVE_CONFIG_H -I. -I../include     -g -O0 -MT hpuifilter.o -MD -MP
-MF .deps/hpuifilter.Tpo -c -o hpuifilter.o hpuifilter
.c
--------------------------------------------------------------

* Macros
  - Please use macros:
    https://fedoraproject.org/wiki/Packaging/RPMMacros
    For example, etc must be %{_sysconfdir}, /var must be %{_localstatedir}
    or so.

* Scriptlet related dependency
  - Please add "Requires(pre)" or so according to
    https://fedoraproject.org/wiki/Packaging/UsersAndGroups

* Timestamps
  - When using "cp" or "install" commands, please add "-p" option to keep timestamps
    on installed files.

* /sbin/ldconfig call
  - The rebuilded binary rpm does not contain any libraries under the default
    ld search path, so calling /sbin/ldconfig on scriptlets is not needed.

* Invalid directory
  - using /var/_local_ is forbidden. Please use %_localstatedir/%name or so.

* Directory ownership issue
  - Please make it sure that all directories created when installing a package
    are correctly owned by the rpm.
    For example, the directories %{_sysconfdir}/rancid/ and %{_datadir}/rancid/
    are not owned by any packages.


Comment 4 Mamoru TASAKA 2008-07-24 17:38:43 UTC
ping?

Comment 5 Åge Olai Johnsen 2008-08-01 08:36:14 UTC
I'm sorry, i've been on a short vacation. Will ship a new one out by this 
weekend.

Comment 6 Mamoru TASAKA 2008-09-10 07:20:40 UTC
ping again?

Comment 7 Åge Olai Johnsen 2008-09-14 19:05:47 UTC
Forgive me for not being present at all here! This project have been a part of my work and I had to prioritize.. I hope i will be able to update it during this week.

Comment 8 Åge Olai Johnsen 2008-09-23 20:19:42 UTC
I've been busy the last couple of weeks but i've squeeze in a update here.

Still working on the optflag-part.

New spec and srpm:
http://files.thaumaturge.org/users/dante/rancid/rancid-0.3.spec
http://files.thaumaturge.org/users/dante/rancid/rancid-2.3.2-0.3.a8.fc9.src.rpm

Comment 9 Mamoru TASAKA 2008-09-26 18:49:30 UTC
Created attachment 317827 [details]
patch to honor cflags correctly

For 2.3.2-0.3.a8

* spec file name
  - First of all, the name of the spec file should be "rancid.spec"

* About optflags issue
  - Applying the patch attached will solve this issue.

* SourceURL
  - Source0 must be written with full URL:
    https://fedoraproject.org/wiki/Packaging/SourceURL

* Perl module dependency
  - The explicit dependencies perl(LockFile::Simple), perl(Mail::Mailer) can be
    removed. These dependencies are automatically found and added to binary
    rpm by rpmbuild itself.

* Macros
  - Please use macros for standard directories. /etc should be %{_sysconfdir}.

* Timestamp
  - Please consider to use
--------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
--------------------------------------------------------
    to keep timestamps on installed files. This method usually works for
    Makefiles generated by recent autotools.

* Directory ownership issue
  - Again please check if all directories which are created when
    this rpm is installed are correctly owned by this package.
    For example, the directory %{_sysconfdir}/%{name}/, 
    %{_datadir}/%{name}/ are not owned by any packages:

    https://fedoraproject.org/wiki/PackagingDrafts/UnownedDirectories#Common_Mistakes

Then:
* Namespace conflict in %_bindir
  - This package tries to install so many files under %_bindir, and
    some of the files have rather generic names (like {a,b,c,...}login).

    Please
    - Move files to the directory specific to this rpm (e.g. %{_libdir}/%{name})
    - Or rename binaries so that there names can easily be distinguished from
      binaries from other packages (like rancid-alogin or so)

    to avoid namespace conflict. Currently I have:
----------------------------------------------------------
# LANG=C rpm -ivh rancid-2.3.2-0.3.a8.fc10.1.i386.rpm 
Preparing...                ########################################### [100%]
        file /usr/bin/par from install of rancid-2.3.2-0.3.a8.fc10.1.i386 conflicts with file from package par2cmdline-0.4-14.fc9.i386
----------------------------------------------------------

    Note that I have not installed the rebuilt binary rpm yet because of
    this conflict.

Comment 10 Åge Olai Johnsen 2008-09-30 12:27:00 UTC
Thank you for the _thorough_ feedback and the makefile. 

I've uploaded a new version:
http://files.thaumaturge.org/users/dante/rancid/rancid.cron
http://files.thaumaturge.org/users/dante/rancid/rancid-2.3.2-0.4.a8.fc9.i386.rpm

Namespace-conflict:
I've moved the binaries to %{_libdir}/%{name} and symlinked in some of them to %{_bindir}.

Directory ownership:
%{_sysconfdir}/%{name}/ should be owned by this command "%attr(750,%{name},%{name}) %config(noreplace) %{_sysconfdir}/%{name}/" ?

Comment 11 Mamoru TASAKA 2008-09-30 17:10:32 UTC
(Removing FE-Legal regarding the license on the web as obsolete, review follows later)

Comment 12 Mamoru TASAKA 2008-09-30 17:54:19 UTC
For -0.4.a8:

* CFLAGS
------------------------------------------------
export CFLAGS=$RPM_OPT_FLAGS
------------------------------------------------
  - This line is not needed because %configure does this.
    (You can check what %configure does by
     $ rpm --eval %configure )

* Macros in comment
------------------------------------------------
#%{_sysconfdir}-files
------------------------------------------------
  - Use %% in comments (and %changelog) instead of %% to prevent
    maros from being expanded.

* Symlinks
------------------------------------------------
W: symlink-should-be-relative /usr/bin/rancid-cvs /usr/lib/rancid/rancid-cvs
W: symlink-should-be-relative /usr/bin/rancid-run /usr/lib/rancid/rancid-run
W: symlink-should-be-relative /usr/bin/rancid-fe /usr/lib/rancid/rancid-fe
W: symlink-should-be-relative /usr/bin/rancid /usr/lib/rancid/rancid
------------------------------------------------
  - It is requested that all symlinks should be relative (not absolute)
    i.e.
------------------------------------------------
for base in \
	%{name} %{name}-cvs %{name}-fe %{name}-run
	do
	ln -sf ../../%{_libdir}/%{name}/${base} \
		$RPM_BUILD_ROOT%{_bindir}/${base}
done
------------------------------------------------
   is better. Note that with the above lines you don't have to
   "push" to $RPM_BUILD_ROOT%{_bindir} .

* About permission/directory ownership

  * %_sysconfdir/%name
(In reply to comment #10)
> Directory ownership:
> %{_sysconfdir}/%{name}/ should be owned by this command
> "%attr(750,%{name},%{name}) %config(noreplace) %{_sysconfdir}/%{name}/" ?
    - Perhaps you want:
-------------------------------------------------
#%%{_sysconfdir}-files
%attr(750,%{name},%{name}) %dir %{_sysconfdir}/%{name}
%attr(600,%{name},%{name}) %config(noreplace) %{_sysconfdir}/%{name}/*
-------------------------------------------------

  * Directories in filesystem rpm
-------------------------------------------------
%dir %{_bindir}/
%dir %{_mandir}/*/
-------------------------------------------------
    - These directories are owned by filesystem rpm and so should not
      be owned by this rpm.

Comment 13 Mamoru TASAKA 2008-09-30 18:06:55 UTC
By the way:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

Comment 14 Åge Olai Johnsen 2008-09-30 18:26:58 UTC
Thank you for your fast reply. I've updated the spec-file:
http://files.thaumaturge.org/users/dante/rancid/rancid.spec
http://files.thaumaturge.org/users/dante/rancid/rancid-2.3.2-0.5.a8.fc9.i386.rpm

Comment 15 Mamoru TASAKA 2008-10-01 18:20:00 UTC
Well,

%attr(750,%{name},%{name}) %config(noreplace) %{_sysconfdir}/%{name}/*

Would you explain why files (not directories) under %_sysconfdir/%name
should have 0750  permission (i.e. executable permission)?

Also, I will wait for your another review request or your pre-review.

Comment 16 Åge Olai Johnsen 2008-10-08 17:10:38 UTC
The "strange" setting was done in a hurry. I've changed it to something closer to usefull:
http://files.thaumaturge.org/users/dante/rancid/rancid.spec
http://files.thaumaturge.org/users/dante/rancid/rancid-2.3.2-0.6.a8.fc9.i386.rpm

Comment 17 Mamoru TASAKA 2008-10-09 05:40:50 UTC
Now this package itself is good.

So as I wrote in comment 13, I will wait for your another review request or
pre-review of other person's review request.

Comment 18 Mamoru TASAKA 2008-10-18 14:51:58 UTC
ping?

Comment 19 Mamoru TASAKA 2008-10-26 14:06:27 UTC
ping again?

Comment 20 Åge Olai Johnsen 2008-10-30 20:48:01 UTC
Due to "alot of stuff to fix", I will not be able to do any other request/pre-review until next week.

Comment 21 Michael Stahnke 2008-11-04 22:28:18 UTC
When providing new rpms, please provide SOURCE rpms and not i386.

Comment 22 Mamoru TASAKA 2008-11-12 16:41:56 UTC
ping again?

Comment 23 Mamoru TASAKA 2008-11-19 17:53:11 UTC
ping again?

Comment 24 Åge Olai Johnsen 2008-11-24 00:31:30 UTC
Sorry for the enormous delay! 

I've submited a second review request:
Bug 472713 - Review Request: Smokeping - keeps track of your network latency 
https://bugzilla.redhat.com/show_bug.cgi?id=472713

Comment 25 Åge Olai Johnsen 2008-11-24 07:28:41 UTC
And even more sorry that smokeping-package already was made...
I cannot believe that i missed it when i was looking for it!

Comment 26 Mamoru TASAKA 2008-11-24 15:52:02 UTC
Well, although your another review request was closed as duplicate, your
spec file itself seemed in good shape to some extent.

And as this package itself is now good, I will approve this package.
---------------------------------------------------------
          This package (rancid) is APPROVED by mtasaka
---------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
After you request for sponsorship a mail will be sent to sponsor 
members automatically (which is invisible for you) which notifies 
that you need a sponsor. After that, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 9/10, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Comment 27 Mamoru TASAKA 2008-12-07 15:49:45 UTC
ping?

Comment 28 Åge Olai Johnsen 2008-12-09 20:13:16 UTC
Hi! Thank you for your feedback on this package and approving it!
I'm about to start on the process of rebuilding it for import. 

I've just requested sponsorship and my FAS-name is "aageolai".

Comment 29 Mamoru TASAKA 2008-12-10 14:50:12 UTC
Okay, now I am sponsoring you. Please follow "Join" wiki again.

Comment 30 Mamoru TASAKA 2008-12-19 17:57:21 UTC
ping?

Comment 31 Mamoru TASAKA 2009-01-08 15:51:11 UTC
ping again?

Comment 32 Mamoru TASAKA 2009-01-17 13:20:33 UTC
I will close this bug as NOTABUG if no response from the reporter
is received within ONE WEEK.

Comment 33 Mamoru TASAKA 2009-01-25 07:37:27 UTC
Closing. Also I removed my sponsorship on the submitter.

If someone wants to import this package into Fedora, please submit
a new review request and mark this request as a duplicate of the
new one.

Thank you!

Comment 34 Gary T. Giesen 2009-07-16 07:16:56 UTC

*** This bug has been marked as a duplicate of bug 512068 ***


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