Brian Gupta wrote:
> Once more into the breech dear friends...
> http://cr.opensolaris.org/~brandorr/gvim_webrev/
> 

I'll take a stab... BTW. It's great to have
contributions like this from the community.

get your Sun sponsor to allocate a bug
category for you (ie. solaris/utility/gvim),
create a tracking RFE and add it to the
webrev (use $ wx ea).

Remove usr/src/cmd/gvim directory from webrev.

You should merge with the current SFW code
- usr/src/cmd/Makefile, usr/src/pkgdefs/Makefile
- and tools have been updated

Almost every file has minor CDDL/copyright header
nits. Take your time and get all the file headers
right. There are bad examples in the SFW code, so
don't just depend on it to show you the way. Here
are the official header prototypes:
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/

usr/src/cmd/Makefile
- I would expect to see changes to ident string.
- are the files checked into SCCS?

usr/src/cmd/gvim/Makefile.sfw
- CDDL/copyright header
- I might change this:
   26 VER=vim71
   27 # we don't include extra patches from vim community
   28 # (those are not present in this directory)
   29 PATCHES=../vim-patches/7.1.*
- to:
   26 #
   27 # gvim uses the source code from vim in usr/src/cmd/vim
   28 # and produces a SUNWgvim package
   29 #
   30 VER=vim71
   31 PATCHES=../vim-patches/7.1.*

   87         cp ../vim/$(VER).tar.bz2 . && cp -r ../vim/vim-patches .
- can "../vim/vim-patches" be "$(PATCHES)" ?

usr/src/cmd/gvim/METADATA
- add "OSR:     6372 (vim)" before COMMENTS line.
    4 OWNER:          Garrett.Damore at sun.comm
- change comm to com, add a comma blank and your email

usr/src/cmd/gvim/install-sfw
    1 #!/usr/bin/bash
- this is the standard shell
    1 #! /usr/bin/ksh93

   41 # remove the binaries that we dont want
   42 rm -rf ${BINDIR}/egvim
...
   51 rm -rf ${BINDIR}/xxd
- use a for loop to remove binaries

   52
- remove extra blank line

   54 # take care of manpages
   55 rm -fr ${ROOTDIR}/${MAN1DIR}/ggvim.1
   56 rm -fr ${ROOTDIR}/${MAN1DIR}/ggvimdiff.1
- the above isn't needed, since you rm files in for loop below

usr/src/cmd/gvim/solaris.patch
- normally we make the name less general now and related it to
the file we are patching. "Makefile.patch" is a better name.

usr/src/pkgdefs/Makefile
- need to add SUNWgvim to this file, add it to the webrev
- have you tried to build package yet?

usr/src/pkgdefs/SUNWgvim/pkginfo.tmpl
- change this:
   34 NAME="Vi IMproved with GTK2 bindings"
   42 DESC="Vi IMproved with GTK2 bindings"
- to:
   34 NAME="gvim - Vi IMproved with GTK2 bindings"
   42 DESC="gvim - Vi IMproved with GTK2 bindings (7.1)"

usr/src/pkgdefs/SUNWgvim/prototype_com
   49 f none usr/bin/gvim 0755 root bin
- change to 0555

Cheers,
Jim

Reply via email to