Hi, New webrev is at http://cr.opensolaris.org/~spoorthy/conmanupd/
Also, please see inline for updates. Srirama Sharma wrote: > Hi Spoorthy, > > Below are my comments: > > 1. In METADATA file, > - Please remove below line. > 11 PSARC: > - Add BUGTRAQ field. > Done . > 2. Please add the new product's entry in usr/src/cmd/Makefile under > 'COMMON_SUBDIRS' in alphabetical order. This is to ensure that nightly > script builds the new component from the toplevel dir. > Done. It was in alphabetical order before also. > 3. In Makefile.sfw, > - Instead of using two command 'gzip' and 'tar' you could use gtar > with '-z' option. Something like > $(GTAR) xzpf $(TARBALL) --no-same-owner; > - What CFLAGS and CXXFLAGS are you using while building? > - In install target, you could also pass on the VER info as below: > PKGVERS=$(VER) $(SHELL) ./install-sfw > With this you could remove the hard coding of version > information done in install-sfw script. > Done . > 4. usr/src/cmd/conman/conman.1 and usr/src/cmd/conman/conman.conf > - Are these Man pages already available in the source tarball or > are these newly written man pages ? > If they are newly written man pages, then they need to have > CDDL headers, Sun Copyright and the sunman-stability. > If they are available along with source tarball then there is > no requirement to check them into the workspace. Instead you could > just install them from the build dir after adding the sunman-stability. > These files can come from source tarball. Forgot to remove them from here before sending webrev. Now, removed them. > 5. usr/src/cmd/conman/conmand.xml > - The CDDL headers are not in proper format. Please check other > manifest files under '/var/svc/manifest' dir. You could also refer to > recent integration/reviews of pen or openwsmand. > - In line 43, the package name should be SUNWconmanr as below: > 43 <service_bundle type='manifest' name='*SUNWconmanr*:conmand'> > - Manifest file doesn't seem to have the exec_method to > stop/disable the service. Is this intended ? If not, a new exec method > to stop the service needs to be added. > - Also curious to know if this service can be managed by any > normal user or does it require any special privileges ? > Done with the changes. But, Is there any source script to know how to write the exec_method to stop/disable the service ? If yes, please give the pointer. > 6. In usr/src/cmd/conman/install-sfw, > - Please change > 1 #! /usr/bin/bash > to > 1 #!/bin/ksh93 > - As mentioned in comment (3) you could get the PKGVERS info from > Makefile.sfw > - In line 54, svc-conmand is a script and should be installed with > 'S' option and not as a Normal file. Yeah. Its a script. So, I changed it to _install S. I didnt knew difference in M,N,S options in _install. > - From lines 57-60, do you really require 744 permissions ? If not > please change the same. Ok. changed permissions at 57,58,60. Conman is an executable. So, changed permission to 555. > - In lines 51, 57, 58 and 60, you are installing the same file into > different locations. Is this intended ? Yeah. This is intended. The executable need to be present at the locations etc/init.d and etc/logrotate.d and also in bin. > - All files specified from lines 62 to 82 are installed as Manpages > using "_install M". Please use 'N' if they are normal files. Done. > 7. Please add entries for the new packages SUNWconmanu and SUNWconmanr > in usr/src/pkgdefs/Makefile. This change is currently not present in > the webrev. > > 8. usr/src/pkgdefs/SUNWconmanr/,Makefile should be removed. The ,xyz > file gets created when you create a new file using sccs. You should > not be including it in webrev. > Removed > 9. usr/src/pkgdefs/SUNWconmanr/Makefile > - There is no 'ident' string. Please add it. > - The depend file seems to be the default one. So you could remove > the depend file for SUNWconmanr and add "DATAFILES = depend" entry in > the Makefile. > - Remove the extra '#' from line 33. Done > > 10. usr/src/pkgdefs/SUNWconmanr/copyright and > usr/src/pkgdefs/SUNWconmanu/copyright > - Source owner copyright lines are missing. > There is a file called "COPYING" in source tar ball which is a copyright file. I have copied the same as copyright file in SUNWconmanu or SUNWconmanr. Let me know if I need to add some more lines there. > 11. Please remove the file usr/src/pkgdefs/SUNWconmanr/pkginfo from > the ws active list as it gets generated from > usr/src/pkgdefs/SUNWconmanr/pkginfo.tmpl. > I had build the component package. So, its in webrev also. > 12. In usr/src/pkgdefs/SUNWconmanr/pkginfo.tmpl, please replace > 46 CLASSES="manifest" > > with > 46 CLASSES="none manifest" > Also please remove the extra line from the end of the file. > > 12. usr/src/pkgdefs/SUNWconmanu/Makefile > - Please add the 'ident' string to the Makefile. > - Are there no other dependencies other than SUNWconmanr? Please > check by running the dependency checker script. > > 13. Please remove the file usr/src/pkgdefs/SUNWconmanu/pkginfo. > This got created when i build the package with "make install". So, its in webrev also. > 14. In usr/src/pkgdefs/SUNWconmanu/pkginfo.tmpl, please replace > 46 CLASSES="manifest" > > with > 46 CLASSES="none" > Also please remove the extra line from the end of the file. > Done > 15. usr/src/pkgdefs/SUNWconmanu/prototype_com > - In line 49, you seem to be installing 'conmen'. This isn't > present in install-sfw and hence will also be not present in the proto > area. So packaging would fail for the same reason. Please check. > 49 f none usr/bin/conmen 0555 root bin Removed it since its not required to be present in usr/bin. > 16. In almost all the files, CDDL header requires cosmetic changes. > Please follow the format for the CDDL headers and Sun Copyright lines > as specified in the link > http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/ > I had followed the same link to add the CDDL header in makefiles. Let me know where I should change ? > 17. Please make sure all the files in your webrev has comments of the > format "<bugid> <synopsis>" > > 18. Please ensure that you run a clean nightly build of your workspace > before regenerating the webrev and make sure there are no errors in > the nightly build log. Nightly build has completed succesfully ... Thanks Spoorthy > > Thanks, > Srirama > > Spoorthy H.S said the following on Monday 16 March 2009 03:18 PM: >> Hi All, >> >> Conman is the Console Management tool. More info on the tool can be >> found at http://home.gna.org/conman/ >> Here's the webrev .. http://cr.opensolaris.org/~spoorthy/conman22/ >> <http://cr.opensolaris.org/%7Espoorthy/conman22/> >> >> Thanks >> Spoorthy >> >> _______________________________________________ >> sfwnv-discuss mailing list >> sfwnv-discuss at opensolaris.org <mailto:sfwnv-discuss at opensolaris.org> >> http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090324/734e918f/attachment.html>
