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/ Thanks, ~Vivek R. Titarmare From: Srirama.Sharma at Sun.COM [mailto:[email protected]] 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/ 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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090515/33c1bd46/attachment.html>
