Hi Vivek,
Please see comments inline.
Vivek Titarmare said the following on Friday 08 May 2009 03:07 PM:
> Hi Srirama,
>
> Regarding point 2
>
> --cut--
> 2. In usr/src/lib/tipc/patch/Makefile.master.patch, you could probably
> change
>
> 15 -TIPC_LIBDIR = $(TIPC_ROOTDIR)/lib
> 16 +TIPC_HDRDIR = usr/include
>
>
> to something like
>
> 15 -TIPC_LIBDIR = $(TIPC_ROOTDIR)/lib
> 16 +TIPC_HDRDIR = usr/include*/net/tipc*
>
>
> and get rid of below patches and may be even more patches. Please check.
> usr/src/lib/tipc/patch/demo.benchmark.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.connection_demo.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.hello_world.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.multicast_demo.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.stream_demo.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.topology_subscr_demo.Makefile.com.patch
>
> [VIVEK] I tried changing the path, but that did not work.
Well. I always thought that the above changes I suggested would work.
So now I downloaded the complete patch from your webrev and built it
myself to see what the issue was. The build went through seamlessly
after doing the above suggested changes and getting rid of those 25 patches.
The changes that you may want to do to see it building are
- just retain "patch/Makefile.patch" and
"patch/Makefile.master.patch" (with the changes I suggested)
and
- Remove rest of the 25 patches from the workspace.
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
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
3. In Makefile.sfw, Is it not required to set PATH variable after doing
an env - ? Am sure it will break looking for dmake.
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.
5. Since tipc-config is a tool, the man page should not be under Section 3.
6. Isn't tipc distributed under a dual BSD/GPL license ? I just see BSD
mentioned in your METADATA and copyright files.
Thanks,
Srirama
> The paths are also required at the compile time for including some of the
> header files. I had
> to keep the paths intact and also the patches. Let me know if you have any
> other solution to this issue.
>
> Also, the webrev is updated with all the changes in the same location.
> (http://cr.opensolaris.org/~vivekrt/6833850-tipc/ )
>
> Thanks,
> ~Vivek R. Titarmare
>
>
>
>
> -----Original Message-----
> From: Vivek Titarmare [mailto:Vivek.Titarmare at Sun.com]
> Sent: Thursday, May 07, 2009 4:17 PM
> To: 'Srirama.Sharma at Sun.COM'
> Cc: 'sfwnv-discuss at opensolaris.org'
> Subject: RE: [sfwnv-discuss] Request code review for "tipc"
>
>
> Thanks for the review Srirama, Pl. check my comments [INLINE]
>
> Webrev would be followed shortly.
>
> Thanks,
> ~Vivek R. Titarmare
>
> -----Original Message-----
> From: Srirama.Sharma at Sun.COM [mailto:Srirama.Sharma at Sun.COM]
> Sent: Wednesday, May 06, 2009 4:21 PM
> To: Vivek Titarmare
> Cc: sfwnv-discuss at opensolaris.org
> Subject: Re: [sfwnv-discuss] Request code review for "tipc"
>
> Hi Vivek,
>
> Below are few more comments:
>
> 1. In Makefile.sfw,
> - Instead of calling out two commands - gzip and gtar, you could
> just use one command i.e gtar with 'z' option to unzip the tarball.
> - "install_h" target is missing. I presume the nightly would fail
> without this target. Please check.
>
> [VIVEK] done
>
> 2. In usr/src/lib/tipc/patch/Makefile.master.patch, you could probably
> change
>
> 15 -TIPC_LIBDIR = $(TIPC_ROOTDIR)/lib
> 16 +TIPC_HDRDIR = usr/include
>
>
> to something like
>
> 15 -TIPC_LIBDIR = $(TIPC_ROOTDIR)/lib
> 16 +TIPC_HDRDIR = usr/include*/net/tipc*
>
>
> and get rid of below patches and may be even more patches. Please check.
> usr/src/lib/tipc/patch/demo.benchmark.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.connection_demo.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.hello_world.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.multicast_demo.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.stream_demo.Makefile.com.patch
> usr/src/lib/tipc/patch/demo.topology_subscr_demo.Makefile.com.patch
>
> [VIVEK] done
>
> 3. Are 64 bit tipc-config binaries not required ? If they do, then
> please make required modifications to prototype_i386 and prototype_sparc
> files.
>
> 4. Also does 'tipc-config' have a man page ?
>
> [VIVEK] yes. It does, however this was not the part of delivery. I will
> create a man page for the same and upload it with the webrev.
>
> Thanks,
> Srirama
>
>
> Vivek Titarmare said the following on Wednesday 29 April 2009 06:54 PM:
>
>> I have posted a webrev for package "tipc" which I am porting to Neveda
>> and would like to request a code review. Please see below link
>>
>>
>>
>> http://cr.opensolaris.org/~vivekrt/6833850-tipc/
>> <http://cr.opensolaris.org/%7Evivekrt/6833850-tipc/>
>>
>>
>>
>>
>>
>> Thanks,
>>
>> ~Vivek R. Titarmare
>>
>>
>>
>>
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090508/8350265b/attachment.html>
