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.
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.
-----------
I have uploaded the webrev for tipc at
http://cr.opensolaris.org/~vivekrt/6833850-tipc/
Let me know if anything is missing in the same.
Thanks,
~Vivek R. Titarmare
From: Srirama.Sharma at Sun.COM [mailto:[email protected]]
Sent: Friday, May 08, 2009 7:19 PM
To: Vivek Titarmare
Cc: sfwnv-discuss at opensolaris.org
Subject: Re: [sfwnv-discuss] Request code review for "tipc"
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:[email protected]]
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:[email protected]]
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/>
<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/20090511/a5eb2978/attachment.html>
