Hi Spoorthy, Below are my comments.
1. METADATA - There are few fields (URL, SUPPORT and BUGTRAQ) missing in METADATA. Please follow the rules specified in the link below : http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines - License should be specified as GPLv2 - OSR number should be added. - Generally porting issues or any notes have to be specified in COMMENTS field. So you could change 9 COMMENTS: "ConMan is a serial console management program designed to support a large number of console devices and simultaneous users" to 9 COMMENTS: 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. 3. usr/src/cmd/conman/Makefile.sfw - The CDDL header needs to be modified. - Version and package name information (VER) could be obtained from the METADATA file as specified below, instead of hard coding the same VER=$(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) - Please change 'env ' to 'env - ' in the target '$(VER)/config.status" - In general, It is advisable to use $(SHELL) instead of $(SH) to ensure predictability of which shell will get called. You may want to call configure as "$(SHELL) ./configure $(CONFIGURE_OPTIONS)" for the same reason. - Please change 59 $(VER)/configure: $(VER).tar.gz to 59 $(VER)/configure: $(TARBALL) - If you intend to use /usr/sfw/bin/gtar then you could use it with "-z" option to unzip the tarball instead of doing the same using 2 commands (/usr/bin/gzip -dc and /usr/sfw/bin/gtar ) 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 in them into the workspace. Instead you could just install them from the build dir after adding the sunman-stability. 5. usr/src/cmd/conman/conmand.xml - The manifest file needs to have CDDL header. - Please change the copyright year to 2009. - In line 17, the package name needs to be corrected. 17 <service_bundle type='manifest' name='*SUNWcsr*:conmand'> - Please check if the man page section in the manifest file (i.e 1M) is right. - 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. 6. usr/src/cmd/conman/install-sfw - CDDL header needs to be modified. Also is line 23 required ? 23 # /* Portions Copyright 2007 ShivaKumar GN */ - Sun Copyright Year should be 2009. - please use ksh93 as specified below Roland Mainz wrote: > use /usr/bin/ksh93 or /usr/bin/bash for install-sfw* > and add a $ set -o errexit # at the beginning and > replace ". ${SRC}/tools/install.subr" with > "source ${SRC}/tools/install.subr" (the idea is to > catch failures in the script and abort it at that point, > right now the script will just continue) - you could pass on PKGVERS variable to install-sfw from Makefile.sfw instead of hard coding it. - In line 54 , do you require executable permission for the manifest file ? If not please change it to 444 7. usr/src/cmd/conman/svc-conmand - CDDL header is missing and the Copyright year should be 2009. - Please use '/usr/bin/ksh93 ' and change 16 . /lib/svc/share/smf_include.sh to 16 source /lib/svc/share/smf_include.sh - The comment in line 9 mentions that '/etc/conmand.conf' is required to be present on the system to start conmand daemon. But you are just checking if the file is readable in line 18. you could change line 18 to something like below : if [ ! -rf /etc/conman.conf ]; then echo "No /etc/conman.conf" exit $SMF_EXIT_ERR_CONFIG fi - Also at the end you could change 25 exit 0 to 25 exit $SMF_EXIT_OK 8. usr/src/pkgdefs/Makefile needs to have an entry of the new package name SUNWconman. This change is currently not present in the webrev. 9. usr/src/pkgdefs/SUNWconman/Makefile - Please correct the CDDL header and Copyright year. 10. usr/src/pkgdefs/SUNWconman/depend - The depend file appears to be the same as the common depend file. Please check if there are any product specific dependencies which needs to be added here. If there are no dependencies then you could remove this depend file and use the common depend file by adding "DATAFILES=depend" in the Makefile. 11. Please remove the file usr/src/pkgdefs/SUNWconman/pkginfo from the ws active list as it gets generated from usr/src/pkgdefs/SUNWconman/pkginfo.tmpl. 12. In usr/src/pkgdefs/SUNWconman/pkginfo.tmpl, you need to add 'manifest' keyword into CLASSES as you would be installing a svc manifest file. 13. usr/src/pkgdefs/SUNWconman/prototype_com - CDDL header and the Copyright year needs to be corrected. - Line 47, there is an extra '='. please remove the same. 47 f none usr/bin/conman= 0555 root bin - In line 48, Is it conmen or conman ? please check. - From line 50-74, these files are not getting installed by install-sfw script. So if you try to create the packge, it could exit with an error. - In line 88, the permissions should be same as the one specified in install-sfw script. (i.e it should be 555). Also the configuration files and smf changes should get into a root package (something like SUNWconmanr) - From line 89-94, these files are not getting installed by install-sfw script. Also I guess you should be installing the svc manifest and method files instead of these. 14. usr/src/pkgdefs/SUNWconman/prototype_i386 and usr/src/pkgdefs/SUNWconman/prototype_sparc - Correct the CDDL header and the Copyright year. 15. please do wx redelget to collapse new files to their initial 1.1 delta. Also, I suggest you to create the final webrev only after a clean nightly build is complete and the packages are tested. Thanks, Srirama Spoorthy H.S said the following on Thursday 19 February 2009 12:01 PM: > Hi, > > I am porting 'conman'. ConMan is a Console Management program. More > info on the tool can be found at http://home.gna.org/conman/ > > Webrev is at http://cr.opensolaris.org/~spoorthy/conman2/ > > Thanks > Spoorthy > _______________________________________________ > sfwnv-discuss mailing list > 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/20090219/4de23f7e/attachment.html>
