Hi Srirama, Pl. see my comments INLINE.
Also the webrev is uploaded with the updates at http://cr.opensolaris.org/~vivekrt/6833850-tipc/ Thanks, ~Vivek R. Titarmare -----Original Message----- From: Srirama.Sharma at Sun.COM [mailto:[email protected]] Sent: Tuesday, May 12, 2009 5:57 PM To: Vivek Titarmare Cc: sfwnv-discuss at opensolaris.org Subject: Re: [sfwnv-discuss] Request code review for "tipc" Hi Vivek, The latest webrev mostly looks good. Few comments below: - In Makefile.sfw, since you are now installing all files through install-sfw script, do you need below lines ? 56 (cd $(CODE_PATH)/$(VER)/usr/src ; \ 57 env - "SRC=$(CODE_PATH)/$(VER)/usr/src" \ 58 "MAKEFLAGS=e" \ 59 "ROOT=$(CODE_PATH)/$(VER)/proto" \ 60 $(MAKE) release) [VIVEK] done Also please check indentation at line 51. 50 for i in $(PATCH_FILES) ; do \ 51 $(GPATCH) -p0 < $${i} ; \ 52 done ; - In Install-sfw, you are just copying the binaries using cp command. It is advisable to use _install subroutine defined in the SFW workspace to install binaries. The subroutine _install also strips the binaries while installation. [VIVEK] done. Also there is no install-sfw-64 script. How are 64 bit version files getting installed ? [VIVEK] added. - AFAIK, the .8 man pages need to be installed under .1m section on Solaris. Please see below link: http://developers.sun.com/solaris/articles/man_pages.html [VIVEK] changed - In copyright file, the sun disclaimer is missing. [VIVEK] added. - It appears that you are using default depend file. If this is the case, you could remove the depend file and add "DATAFILES = depend" entry in SUNWtipc/Makefile. [VIVEK] added - Also please see inline reply for previous comments. Vivek Titarmare said the following on Monday 11 May 2009 08:43 PM: > > Hi Srirama, > > Thanks for the review. Pl. see my comments [INLINE] > > --cut- > > Also below are few more comments on the latest webrev: > > 1. As per the heads-up mail from Norm, below changes are required to > the METADATA file > - Remove PROGRAM field > - Provide a space at line 8. Else there could be a parsing error > during build. So please change > > 8 SOURCE_DOWNLOAD:http://opensolaris.org/os/project/tipc/files/TIPC_Solaris_1_ 7_6_rc1-2008-07-08_SRC.tar.bz2 > > to > > 8 SOURCE_DOWNLOAD: http://opensolaris.org/os/project/tipc/files/TIPC_Solaris_1_7_6_rc1-2008-07- 08_SRC.tar.bz2 > > [VIVEK] done. > > > 2. In your METADATA file, the SOURCE_DOWNLOAD field refers to a bz2 > where as the source tarball present in the webrev is a tar.gz > In community if the tarball is available in tar.bz2 format, I suggest > you to use the same instead of untarring it and again creating a tar.gz > > [VIVEK] changed. > > > 3. In Makefile.sfw, Is it not required to set PATH variable after > doing an env - ? Am sure it will break looking for dmake. > > [VIVEK] No, we have used SRC which takes care of the PATH and also > verified with dmake it is working fine. > When env - is done, all previously set env variables will be cleared. So I was always under the impression that we need to set PATH too. Something like "PATH=$(SFW_PATH)". If it still works for you without PATH being set then am okay with it. [VIVEK] I have added the PATH. > > 4. In Makefile.sfw, you are using the makefile install target of the > component itself. So suggest you to check if the permissions of files > in the proto area match with those in the prototype_com file using > 'protofix' tool. > > [VIVEK] changed the install-sfw file to match the requirements. > > > 5. Since tipc-config is a tool, the man page should not be under > Section 3. > > [VIVEK] changed to section 8. > > > 6. Isn't tipc distributed under a dual BSD/GPL license ? I just see > BSD mentioned in your METADATA and copyright files. > > [VIVEK] added GPL and also updated copyright file. > Please check the OSR to make sure the same is reflected there. Thanks, Srirama
