On Fri, Sep 26, 2008 at 2:16 PM, Jim Walker <James.Walker at sun.com> wrote:
> 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).

Mmm.. this might be a problem. Garrett has indicated that while he can
sponsor me for the PSARC case, I will need a sponsor from the SFWNV
community to get other stuff done. Could someone who is familiar with
this process please help with sponsorship?

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

Done.

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

Done. I have downloaded the latest SFW src tarball (
sfw-src-b99-20080918.tar.bz2 ) and I am
using that as the base for the builds.

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

Fixed. Thanks for the tip!

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

Could you explain about this in a little more detail please? What
exactly does the "ident" string do? I have kept it the same as it was
for SUNWvim

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

Done.

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

No. That should not be $(PATCHES). $PATCHES refers to the patches dir
residing in the "vim" directory, which are copied over to the current
dir. Deleting $(PATCHES) will clear up that particular directory.

> 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

Done.

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

Done.

>  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

Done.

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

Renamed.

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

Yes. I have used the pkginfo file to build a SUNWgvim.pkg locally here.

> 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)"

Done.

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

Done.

> Cheers,
> Jim
>

-- 
- Brian Gupta

http://opensolaris.org/os/project/nycosug/

http://www.genunix.org/wiki/index.php/OpenSolaris_New_User_FAQ

New York City user groups calendar:
http://www.google.com/calendar/embed?src=nycusergroups%40brandorr.com&ctz=America/New_York

Reply via email to