On 04/13/09, Raymond Xiong wrote:
> On 04/03/09, Paul Cunningham wrote:
> > Raymond,
> >
> > See my comments below ...
>
> Paul, thanks a lot for your comments and sorry for the delay in
> my reply (I got a badly fever and was off most time of the last
> time).
>
> Please reload the webrev:
>
> http://cr.opensolaris.org/~rayx/ejabberd_final/
>
> All your comments make sense to me (although I didn't expect
> you could find so many :), see more below...
>
> > Paul
> >
> > Raymond Xiong wrote:
> > >
> > >Please help review the code to integrate ejabberd:
> > >
> > > http://cr.opensolaris.org/~rayx/ejabberd_final/
> > >
> > >The ARC case and discussion is at:
> > >
> > > https://opensolaris.org/jive/thread.jspa?messageID=243643
> >
> > === Start of Comments ====
> >
> > 1. usr/src/Targetdirs
> > Move your /var/... stuff to be with the other /var
> > stuff after line ...
> > 1118 /var/apache2/2.2 \
> > (I know its all jumbled but its probably better not
> > to make it worse :-) )
>
> Changed. I tried to find a proper place last time and failed to
> figure out the pattern. :)
>
> > 2. usr/src/cmd/ejabberd/METADATA
> > Add missing SRC:, SUPPORT: and BUGTRAQ: lines, see ...
> > "http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines";
>
> Added.
>
> > 3. usr/src/cmd/ejabberd/Makefile.sfw
> > & usr/src/cmd/ejabberd/install-sfw
> > & usr/src/pkgdefs/SUNWejabberdu/Makefile
> > & usr/src/pkgdefs/SUNWejabberdu/depend
> > & and the others that are wrong
> > Cosmetic: remove the double-space in the CDDL header
> > lines so that it conforms to prototypes at ..
> > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/";
>
> Removed extra space in all files that have CDDL header.
>
> > 4. usr/src/cmd/ejabberd/Patches/configure.patch
> > Typo in ...
> > 1 /usr/bin/tail on Soalris doesn't support "-n" option.
>
> Corrected.
>
> > 5. usr/src/cmd/ejabberd/Patches/ejabberdctl.template.patch
> > Just a thought: would it be better to patch it to 'bash'
> > rather than 'ksh' (as that's what 'sh' would be on linux).
>
> Changed. Yes, that make senses.
I just noticed changing ksh to bash have a side effect - it introduced
dependency to SUNWbash package. Since the script is very simple
and doesn't use any bash-special feature, I will change it back
to ksh if you don't object.
Thanks,
Raymond
> > 6. usr/src/cmd/ejabberd/install-sfw
> > Change as follows:
> >
> > > Add #!/usr/bin/ksh93 at top 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)
>
> Changed.
>
> > 7. usr/src/cmd/ejabberd/sunman-stability
> > You probably need to add SUNWejabberdr to the line ..
> > 20 Availability SUNWejabberd
> > and change SUNWejabberd to SUNWejabberdu
> >
> > 8. usr/src/common/rbac/auth_attr
> > Might be better to try and keep this alphabetical if
> > possible, so add your stuff higher up the list.
>
> I modified exec_attr and prof_attr to move ejabberd entry higher
> up in the list, but I still left ejabberd entry at the end of
> auth_attr because existing entries in that file seem not follow
> apphabetical order at all.
>
> > Cosmetic: fix the top of the file so it conforms to ..
> > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/";
> > and in ..
> > usr/src/common/rbac/exec_attr
> > usr/src/common/rbac/prof_attr
>
> Removed the extra "#" lines in CDDL header.
>
> > 9. usr/src/pkgdefs/SUNWejabberdr/depend
> > Does a root package really have these dependencies? It
> > should probably use the default depend
>
> Corrected.
>
> > 10. usr/src/pkgdefs/SUNWejabberdu/copyright
> > Do you need to add the Sun disclaimer at the top?
> >
> > Add the source owner copyright lines, extracted from
> > the source files in the unpacked tarball, at the topish
>
> All were added.
>
> > 11. usr/src/pkgdefs/SUNWejabberdu/pkginfo.tmpl
> > Add the pkg version number at the end of the DESC=
> > line, eg.
> > DESC="..... (2.0.0)"
>
> Changed.
>
> Thanks Again,
> Raymond
>
> > === End of Comments ======
> > --
> > ----------------------------------------------------------------------
> > Paul Cunningham
> > Software Engineer
> > Tadpole Business Unit
> _______________________________________________
> sfwnv-discuss mailing list
> sfwnv-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss
--
Regards,
Raymond
