Bug 517859 - Review Request: vmpsd - vmpsd is a GPL implementation of Cisco Systems' VMPS
Summary: Review Request: vmpsd - vmpsd is a GPL implementation of Cisco Systems' VMPS
Keywords:
Status: CLOSED ERRATA
Alias: None
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:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-08-17 14:15 UTC by Colin Coe
Modified: 2009-09-28 18:23 UTC (History)
4 users (show)

Fixed In Version: 1.4.01-6.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-09-11 14:33:43 UTC
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Colin Coe 2009-08-17 14:15:46 UTC
Spec URL: http://members.iinet.net.au/~coec/vmpsd.spec
SRPM URL: http://members.iinet.net.au/~coec/vmpsd-1.4.01-1.src.rpm
Description: 
VMPS (VLAN Management Policy Server) is a way of assigning switch ports to 
specific VLANs based on MAC address of connecting device. OpenVMPS is a GPL 
implementation of VMPS.

Comment 1 Stjepan Gros 2009-08-18 20:53:28 UTC
I looked a bit on your package and I have few comments/questions. You are generating init scripts in spec file. Isn't it better to distribute them along with source?

Furhtermore, I believe you should add %preun section to spec file that will try to stop the service before uninstaling it.

There is an error reported by rpmlint when run on the binary package:

$ rpmlint vmpsd-1.4.01-1.fc11.x86_64.rpm 
vmpsd.x86_64: W: summary-not-capitalized vmpsd is a GPL implementation of Cisco Systems' VMPS
vmpsd.x86_64: W: name-repeated-in-summary vmpsd
vmpsd.x86_64: E: init-script-without-chkconfig-postin /etc/rc.d/init.d/vmpsd
vmpsd.x86_64: W: service-default-enabled /etc/rc.d/init.d/vmpsd
1 packages and 0 specfiles checked; 1 errors, 3 warnings.


Trying to build package in mock was successful.

Comment 2 Colin Coe 2009-08-20 14:14:05 UTC
Hi Stjepan Gros, and thanks for the review.

My understanding is that it is OK to put the scripts in the spec file but I will put it in the source if I'm wrong.

I've corrected to %preun and tidied up the rpmlint output.

rpmlint /home/coec/rpmbuild/SRPMS/vmpsd-1.4.01-1.fc11.src.rpm /home/coec/rpmbuild/RPMS/i586/vmpsd-1.4.01-1.fc11.i586.rpm vmpsd.spec 
vmpsd.i586: W: service-default-enabled /etc/rc.d/init.d/vmpsd
2 packages and 1 specfiles checked; 0 errors, 1 warnings.

I should have advised that I need a sponsor also.

I've updated the spec and src.rpm.

Spec URL: http://members.iinet.net.au/~coec/vmpsd.spec
SRPM URL: http://members.iinet.net.au/~coec/vmpsd-1.4.01-2.src.rpm

Comment 3 Colin Coe 2009-08-21 04:19:50 UTC
koji build --scratch dist-f11 ../SRPMS/vmpsd-1.4.01-2.fc11.src.rpm 
Uploading srpm: ../SRPMS/vmpsd-1.4.01-2.fc11.src.rpm
[====================================] 100% 00:00:06 195.38 KiB  29.98 KiB/sec
Created task: 1619537
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=1619537
Watching tasks (this may be safely interrupted)...
1619537 build (dist-f11, vmpsd-1.4.01-2.fc11.src.rpm): open (ppc5.fedora.phx.redhat.com)
  1619541 buildArch (vmpsd-1.4.01-2.fc11.src.rpm, i586): open (x86-1.fedora.phx.redhat.com)
  1619538 buildArch (vmpsd-1.4.01-2.fc11.src.rpm, ppc): open (ppc2.fedora.redhat.com)
  1619540 buildArch (vmpsd-1.4.01-2.fc11.src.rpm, ppc64): open (ppc7.fedora.phx.redhat.com)
  1619539 buildArch (vmpsd-1.4.01-2.fc11.src.rpm, x86_64): open (xenbuilder4.fedora.phx.redhat.com)
  1619541 buildArch (vmpsd-1.4.01-2.fc11.src.rpm, i586): open (x86-1.fedora.phx.redhat.com) -> closed
  0 free  4 open  1 done  0 failed
  1619539 buildArch (vmpsd-1.4.01-2.fc11.src.rpm, x86_64): open (xenbuilder4.fedora.phx.redhat.com) -> closed
  0 free  3 open  2 done  0 failed
  1619540 buildArch (vmpsd-1.4.01-2.fc11.src.rpm, ppc64): open (ppc7.fedora.phx.redhat.com) -> closed
  0 free  2 open  3 done  0 failed
  1619538 buildArch (vmpsd-1.4.01-2.fc11.src.rpm, ppc): open (ppc2.fedora.redhat.com) -> closed
  0 free  1 open  4 done  0 failed
1619537 build (dist-f11, vmpsd-1.4.01-2.fc11.src.rpm): open (ppc5.fedora.phx.redhat.com) -> closed
  0 free  0 open  5 done  0 failed

1619537 build (dist-f11, vmpsd-1.4.01-2.fc11.src.rpm) completed successfully

Comment 4 Mamoru TASAKA 2009-08-30 19:15:49 UTC
Some random notes:

* License
  - The license tag should be "GPL+", because no version information
    is given in the tarball (just putting GPLv2 license in the tarball
    does not specify the version of GPL: see below)
    https://fedoraproject.org/wiki/Licensing/FAQ#How_do_I_figure_out_what_version_of_the_GPL.2FLGPL_my_package_is_under.3F

* URL
  - The following is more proper:
    http://vmps.sourceforge.net/

* BuildRequires
  - BR: gcc is redundant.
    https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

* Building source
  - Unless some specialy reason exists, compiling source codes (by "make",
    for example) must be done at %build stage and %install stage should
    do just installing files (by "make install", for example) and so on.
    (i.e. add "make %{?_smp_mflags}" after %configure, also see below)
    https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

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

* Creating scripts by cat in spec file
  - Please avoid this as much as possible, just create a text
    files beforehand, add these as %SOURCEx, and install
    the text files at %install (also see:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

* About scriptlets treatment
  ! Please read:
    https://fedoraproject.org/wiki/Packaging/SysVInitScript
    Some notes:
    - Some Requires(pre) or so is missing.
      https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets
    - Service should not be enabled by default:
      https://fedoraproject.org/wiki/Packaging/SysVInitScript#.23_chkconfig:_line
    - Please use %_initddir
      https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem
    - Consier to add "condrestart" at %postun

* Macros
  - Please use macros properly.
    https://fedoraproject.org/wiki/Packaging/RPMMacros
    - Use %{_sysconfdir}, %{_bindir} and so on.
    - %{_datadir/man} should be %{_mandir}

Comment 5 Colin Coe 2009-09-04 07:57:26 UTC
Thank you for the detailed review.

I believe I have addressed the points you raised and have updated the files.

Spec URL: http://members.iinet.net.au/~coec/vmpsd.spec
SRPM URL: http://members.iinet.net.au/~coec/vmpsd-1.4.01-3.src.rpm  

Please note that this is intended to be build on RHEL5 so some of the macros I can't use.

Thanks again

CC

Comment 6 Mamoru TASAKA 2009-09-04 17:34:19 UTC
For -3:

* initscript treatment
  - Still Requires(post) or so are missing:
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets
    ( See Requires(post) or so item )

  - Again usually service should not be enabled by default.
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#.23_chkconfig:_line
    chkconfig line in vmpsd.init should be:
--------------------------------------------------------------------
# chkconfig: - 92 8
--------------------------------------------------------------------

  - And would you check if "condrestart" is not neeeded?

* SourceURL
  - For sourceforge.net hosted source tarball, please see:
    https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

* %clean section
  - I recommend to write %clean section after %install section.

* make
  - On %build section, "make" is enough, "DESTDIR=$RPM_BUILD_ROOT" is not needed
    ( DESTDIR=foo is needed for "make install" )

  ! Again please add %{?_smp_mflags} if possible:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

* Specifying user/group with "install" command
  - cannot be done as usually rpmbuild is done as non-previledged user.
    So in rpmbuild "-o root -g root" with install fails, this is simply
    not needed.

* Macros
  - Please use macros even in %files section

Comment 7 Colin Coe 2009-09-05 10:03:42 UTC
Thanks for your patience.

Spec URL: http://members.iinet.net.au/~coec/vmpsd.spec
SRPM URL: http://members.iinet.net.au/~coec/vmpsd-1.4.01-4.src.rpm  

I think I've addressed these concerns.

CC

Comment 8 Mamoru TASAKA 2009-09-05 17:42:07 UTC
For -4:

* SourceURL
  - I recommend to use %{name} and %{version} macro in
    SourceURL (especially %{version}), with this you probably
    won't have to change SourceURL when version is upgraded:
    https://fedoraproject.org/wiki/Packaging/SourceURL#Using_.25.7Bversion.7D

* condrestart
  - condrestart should be in %postun:
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets

* Timestamps
  - Please add "-p" option to "install" command
    https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
------------------------------------------------------
install %{S:2} $RPM_BUILD_ROOT%{_initrddir}/%{name}
------------------------------------------------------

* Macros
  - /usr/share/man should be %{_mandir}

* Documents
  - Forgot to mention, however:
  * Please add COPYING{.GPL} to %doc
  * And consider to add ChangeLog, README to %doc.

Comment 9 Colin Coe 2009-09-06 02:25:02 UTC
Thanks for your continued patience

I've addressed your concerns for -4.

Spec URL: http://members.iinet.net.au/~coec/vmpsd.spec
SRPM URL: http://members.iinet.net.au/~coec/vmpsd-1.4.01-5.src.rpm  

CC

Comment 10 Mamoru TASAKA 2009-09-06 16:18:57 UTC
For -5:

* Initscripts (condrestart)
  - condrestart in %post scriptlet is no longer needed ( as
    condrestart is called at %postun )

* Documents
  - Usually the file "INSTALL" is for people installing the software
    by building it by themselves and not needed for people using
    packaged rpm.

Comment 11 Colin Coe 2009-09-07 08:01:15 UTC
Thanks for your continued patience

I've addressed your concerns for -5.

Spec URL: http://members.iinet.net.au/~coec/vmpsd.spec
SRPM URL: http://members.iinet.net.au/~coec/vmpsd-1.4.01-6.src.rpm  

CC

Comment 12 Mamoru TASAKA 2009-09-07 16:14:28 UTC
Well,

* In %changelog, we usually don't add %{?dist} to release number
  Please fix this when you import this package into Fedora CVS

Now:
- This package itself can be approved
- As written in
  http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored
  we usually ask the person who needs sponsorship to either
  submit another review request or do a "pre-review" of other
  person's review request.

  Now you have other review requests and their packaging seems good
  to some extent from a quick glance.

--------------------------------------------------------
    This package (vmpsd) is APPROVED by mtasaka
--------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji)".

Now I am sponsoring you.

If you want to import this package into Fedora 10/11, 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.

Removing NEEDSPONSOR.

Comment 13 Colin Coe 2009-09-08 12:02:36 UTC
New Package CVS Request
=======================
Package Name: vmpsd
Short Description: A GPL implementation of Cisco Systems' VMPS
Owners: coec
Branches: F-10 F-11 EL-5
InitialCC: coec

Comment 14 Kevin Fenzi 2009-09-09 16:20:36 UTC
cvs done.

Comment 15 Colin Coe 2009-09-10 10:57:08 UTC
I'm having troubles with fedora-cvs.  Any ideas what I'm doing wrong here?

---
[coec@rsim cvs]$ ssh-add
Could not open a connection to your authentication agent.
[coec@rsim cvs]$ exec ssh-agent bash
[coec@rsim cvs]$ ssh-add
Enter passphrase for /home/coec/.ssh/id_rsa:
Identity added: /home/coec/.ssh/id_rsa (/home/coec/.ssh/id_rsa)
[coec@rsim cvs]$ fedora-cvs vmpsd
Checking out vmpsd from fedora CVS as coec:
Permission denied (publickey).
cvs [checkout aborted]: end of file from server (consult above messages if any)
[coec@rsim cvs]$
[coec@rsim cvs]$ ls -la ~/.ssh
total 20
drwx------.  2 coec coec 4096 2009-09-10 18:44 .
drwx------. 38 coec coec 4096 2009-09-10 18:21 ..
-r--------.  1 coec coec 1743 2009-09-10 18:44 id_rsa
-rw-r--r--.  1 coec coec  402 2009-09-10 18:44 id_rsa.pub
-rw-r--r--.  1 coec coec  642 2009-09-10 18:31 known_hosts
---

CC

Comment 16 Mamoru TASAKA 2009-09-10 11:26:18 UTC
Well,

- Would you verify that you uploaded your ssh public key to
  FAS (Fedora Account System)? (if not please visit your
  FAS page)

- And would you check if you have installed "fedora-package"
  rpm and execute $ fedora-packager-setup ?

Comment 17 Colin Coe 2009-09-10 12:31:53 UTC
Thankyou for the pointer.

I've done the CVS thing for vmpsd in F-10, F-11 and EL-5.

There was a line in each saying 'Source upload succeeded. Don't forget to commit the new ./sources file'.  How do I do this?

Is there a complete wiki page on all of this.  I'm having trouble finding all the info in one spot.

Thanks again

CC

Comment 18 Mamoru TASAKA 2009-09-10 12:59:29 UTC
(In reply to comment #17)
> There was a line in each saying 'Source upload succeeded. Don't forget to
> commit the new ./sources file'.  How do I do this?

This means that you have to do "cvs commit" after "make new-sources"
or "make upload".  However from cvs-extras-commits list you used
"cvs-import.sh" and with this script it was done automatically.

Now please build this package on koji, and for F-10/11/EL-5 please
submit push requests on bodhi.

Comment 19 Colin Coe 2009-09-10 13:46:01 UTC
OK, build in koji and now attempting to push through dodhi.

Stuck in the package name as it is not autocompleting and also saying 'invalid build'.  The package name I'm giving is 'vmpsd-1.4.01-6.f10' (also tried 'vmpsd-1.4.01-6.fc10') as per http://fedoraproject.org/wiki/PackageMaintainers/Join#Install_the_Client_Tools_.28Koji.29

I'm not deliberated tryig to be dense here.

Thanks again for all of your help.

CC

Comment 20 Mamoru TASAKA 2009-09-10 16:15:31 UTC
Well, actually it seems you have done "make tag", but
you have not built vmpsd with koji yet:
http://koji.fedoraproject.org/koji/packageinfo?packageID=9159

Please actually build the package:
http://fedoraproject.org/wiki/PackageMaintainers/Join#Request_Builds

Comment 21 Fedora Update System 2009-09-11 13:15:20 UTC
vmpsd-1.4.01-6.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/vmpsd-1.4.01-6.fc10

Comment 22 Fedora Update System 2009-09-11 13:15:27 UTC
vmpsd-1.4.01-6.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/vmpsd-1.4.01-6.el5

Comment 23 Fedora Update System 2009-09-11 13:15:31 UTC
vmpsd-1.4.01-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/vmpsd-1.4.01-6.fc11

Comment 24 Colin Coe 2009-09-11 13:18:43 UTC
Mostly done except for f12 which gives me this message:
"vmpsd-1.4.01-6.fc12 not tagged as an update candidate"

Are upcoming Fedora releases treated differently?

CC

Comment 25 Mamoru TASAKA 2009-09-11 14:33:43 UTC
For now you don't have to submit push requests on bodhi for F-12.
Packages rebuilt for F-12 are automatically pushed into rawhide tree
next day.

Closing.

Comment 26 Fedora Update System 2009-09-15 07:36:02 UTC
vmpsd-1.4.01-6.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 27 Fedora Update System 2009-09-15 07:54:30 UTC
vmpsd-1.4.01-6.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 28 Fedora Update System 2009-09-28 18:22:57 UTC
vmpsd-1.4.01-6.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.


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