Bug 517859
Summary: | Review Request: vmpsd - vmpsd is a GPL implementation of Cisco Systems' VMPS | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Colin Coe <colin.coe> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | fedora-package-review, mtasaka, notting, stjepan.gros |
Target Milestone: | --- | Flags: | mtasaka:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 1.4.01-6.el5 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-09-11 14:33:43 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Colin Coe
2009-08-17 14:15:46 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. 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 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 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} 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 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 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 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. 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 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. 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 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. 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 cvs done. 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 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 ? 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 (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. 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 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 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 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 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 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 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. 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. 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. 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. |