Hi Vivek, The updated webrev looks okay to me.
Thanks, Srirama Vivek Titarmare said the following on Friday 15 May 2009 12:01 PM: > > Hi Srirama, > > > > Thanks for the review. The changes are done and the new webrev is > uploaded at the same location. > > http://cr.opensolaris.org/~vivekrt/6833850-tipc/ > <http://cr.opensolaris.org/%7Evivekrt/6833850-tipc/> > > > > Thanks, > > ~Vivek R. Titarmare > > > > > > *From:* Srirama.Sharma at Sun.COM [mailto:Srirama.Sharma at Sun.COM] > *Sent:* Thursday, May 14, 2009 5:42 PM > *To:* Vivek Titarmare > *Cc:* sfwnv-discuss at opensolaris.org > *Subject:* Re: [sfwnv-discuss] Request code review for "tipc" > > > > Hi Vivek, > > The updated webrev looks okay to me. > > Just a few comments. > > 1. In Makefile.sfw, as install-sfw and install-sfw-64 are present, you > may want to remove these lines. > > 57 (cd $(CODE_PATH)/$(VER)/usr/src ; \ > 58 env - "SRC=$(CODE_PATH)/$(VER)/usr/src" \ > 59 "MAKEFLAGS=e" \ > 60 "ROOT=$(CODE_PATH)/$(VER)/proto" \ > 61 PATH=$(SFW_PATH) \ > 62 $(MAKE) release) > > > 2. In install-sfw, > - Did you intend to create link to usr/lib/libtipcsocket.so.1. If > so then please change line 81 as shown below: > > 81 _install L *usr/lib/libtipcsocket.so* ${LIBDIR}/libtipcsocket.so > > to > > 81 _install L *libtipcsocket.so.1* ${LIBDIR}/libtipcsocket.so > > - Also you may want to install the binary tipc-config as an > executable using 'E' option as below > > 85 _install *E* usr/sbin/tipc-config ${SBINDIR}/tipc-config 555 > > > 3. In install-sfw-64, same comments as in (2) > > 4. In tipc-config.1m, you may want to change the section header to .1m > at line 30 as shown below. > > 30 .TH tipc-config *3* "07 May 2009" "tipc-config 1.1.6" > > to > > 30 .TH tipc-config *1m* "07 May 2009" "tipc-config 1.1.6" > > > Thanks, > Srirama > > Vivek Titarmare said the following on Wednesday 13 May 2009 06:47 PM: > > Hi Srirama, > > Pl. see my comments INLINE. > > Also the webrev is uploaded with the updates at > http://cr.opensolaris.org/~vivekrt/6833850-tipc/ > <http://cr.opensolaris.org/%7Evivekrt/6833850-tipc/> > > Thanks, > ~Vivek R. Titarmare > > -----Original Message----- > From: Srirama.Sharma at Sun.COM <mailto:Srirama.Sharma at Sun.COM> > [mailto:Srirama.Sharma at Sun.COM] > Sent: Tuesday, May 12, 2009 5:57 PM > To: Vivek Titarmare > Cc: sfwnv-discuss at opensolaris.org <mailto: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 > >
