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)
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.
Also there is no install-sfw-64 script. How are 64 bit version files
getting installed ?
- 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
- In copyright file, the sun disclaimer is missing.
- 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.
- 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.
>
> 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
