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.
> 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
