Hi Paul,
Thanks a lot for quick reply. A latest webrev had uploaded at the
same link:
http://cr.opensolaris.org/~alz/bittorrent/
My answers are added below, waiting for your advice.
Thanks
- Alex
On Jun 8, 2009, at 5:50 PM, Paul Cunningham wrote:
> Alex,
>
> See comments below ...
>
> Paul
>
> alex zhang wrote:
>> Thanks Lizhong for review.
>> A newer version of webrev at http://cr.opensolaris.org/~alz/bittorrent/
>> Please have a look at it.
>
>>>>
>>>>> From: alex zhang <Huawei.Zhang at Sun.COM>
>>>>> Date: April 1, 2009 4:38:05 PM GMT+08:00
>>>>>
>>>>> Please help me have a code review of opensolaris package
>>>>> porting, the webrev at:
>>>>> http://cr.opensolaris.org/~alz/libtorrent/, which had sent out
>>>>> a few days without any response.
>
>
> 1. usr/src/cmd/rtorrent/METADATA
> & usr/src/lib/libtorrent/METADATA
> These need updating to the new format, see ...
> "http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines";
both cmd/rtorrent/METADATA and lib/libtorrent/METADATA had updated.
>
> 2. Top of Files
> Update the tops of your files so that they conform to that in ..
> "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/
> "
All files had checked and updated
>
> 3. usr/src/cmd/rtorrent/Makefile.sfw
> Explicitly set the 'configure --prefix=' value, using
> $(SHELL) configure $(CONFIGURE_OPTIONS)
>
> Lines ...
> 53 "CXX=$(CC)" \
> 57 ./configure CXX=$(CCC)
> why is CXX= set to $(CC) and why do you then need the CXX=$(CCC)
> after 'configure' ?
in line 52, I set CC=$(CCC), so it isn't conflict with each other.
in configure option, I told him to use sun CC instead of g++
>
> You could modernise the makefile and use the inbuild tarball
> uncompress and patching, see cups/Makefile.sfw
>
> Line ...
> 71 cd ..
> I don't think you need this
removed
>
> Lines ...
> 67 autoheader; \
> 68 libtoolize --automake --copy --force; \
> 69 automake-1.10; \
> 70 autoconf; \
> do you need to run these ?
since one patch file change the "configure.ac" and "Makefile.am", I
think it is safer
to run these.
>
> 4. usr/src/cmd/rtorrent/install-sfw
> & usr/src/lib/libtorrent/install-sfw
> & usr/src/lib/libtorrent/install-sfw-64
> Pass the VERS= ( and/or VERSDIR=) in from the Makefile.sfw
change
VERSDIR=$(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh)?
>
> 5. usr/src/lib/libtorrent/Makefile.sfw
> Lines (and other places) ..
> 49 "CC=$(CCC)" \
> 50 "CXX=$(CC)"
> why is CC equal $(CCC) and CXX equal $(CC) ?
>
> Lines ...
> 83 "CXX=$(CC)"
> 88 ./configure CXX=$(CCC)
> do you need the CXX=$(CCC) on the configure line
same as question 3.
> Line ..
> 69 (cd $(VER); env DESTDIR=$(DESTDIR)
> change 'env ' to 'env - '
changed
>
> You could modernise the makefile and use the inbuild tarball
> uncompress and patching, see cups/Makefile.sfw
>
> Lines ..
> 108 autoheader; \
> 109 libtoolize --automake --copy --force; \
> 110 automake-1.10; \
> 111 autoconf; \
> do you need to run these ?
see answer 3
>
> Line ...
> 112 cd ..)
> I don't think this is needed.
done.
> 6. usr/src/lib/libtorrent/install-sfw
> & usr/src/lib/libtorrent/install-sfw-64
> Apply the following ...
> Roland Mainz wrote:
> > use /usr/bin/ksh93 for install-sfw* and add a
> > $ set -o errexit # at the beginning and replace
> > ". ${SRC}/tools/install.subr" with
> > "source ${SRC}/tools/install.subr" (the idea is to catch
> > failures in the script and abort it at that point,
> > right now the script will just continue)
previous I use "source", but it always report "can not find the
command source"
I don't know if it is cause by Makefile "$(SH) ./install-sfw"
>
> 7. usr/src/lib/libtorrent/install-sfw
> Lines ...
> 43 # install headers
> 44 mkdir -p ${INCDIR}/torrent
> 45 mkdir -p ${INCDIR}/torrent/data
> 46 mkdir -p ${INCDIR}/torrent/peer
> add these dirs to the Targetdirs files and remove the 'mkdir's
> from here
I am not very clear, just remove them? Please help me have a look at
the latest webrev if it is ok.
>
> 8. libtorrent.3 and install-sfw
> I don't think this has the sunman stability stuff in it when
> installed ?
I just write according to libsigsegv/sigsegv.3, since I had provided a
man page, need I provide
sunman-stability file?
>
> 9. patch files
> Note: I haven't looked as any of these
>
> 10. usr/src/pkgdefs/SUNWlibtorrent/copyright
> & usr/src/pkgdefs/SUNWrtorrent/copyright
> Add the src owner copyright lines, after the sun disclaimer,
> extracted from the unpacked tarball files. See example in ..
> "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyright
>
> "
added
>
> 11. usr/src/pkgdefs/SUNWlibtorrent/pkginfo.tmpl
> & usr/src/pkgdefs/SUNWrtorrent/pkginfo.tmpl
> Add src pkg version at end of DESC= line, eg ...
> DESC="............ (x.x.x)"
done
>
> END
> --
> ----------------------------------------------------------------------
> Paul Cunningham
> Software Engineer
> Tadpole Business Unit
