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:Srirama.Sharma at Sun.COM]
> 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/20090514/7fb14067/attachment.html>
