I think I have incorporated everyone's comments. Please let me know if I missed anything. (Latest webrev) http://cr.opensolaris.org/~brandorr/gvim-webrev/
Thanks, Brian On Thu, Sep 18, 2008 at 4:06 PM, Brian Gupta <brian.gupta at gmail.com> wrote: > On Tue, Sep 16, 2008 at 9:29 AM, Paul Cunningham > <paul.cunningham at tadpole.com> wrote: >> Note: changed 'Subject' line >> >> Here are some comments to start with ... > > Thanks. See inline replies. I am not gonna upload the next webrev > until ALL the issues you listed are resolved. (unless you suggest > otherwise). > >> Paul >> >> Brian Gupta wrote: >> ... cut ... >>> >>> I guess that mean I need a code review. Here is the upload: >>> http://cr.opensolaris.org/~brandorr/firstreview/webrev/ >>> >> >> You have no packaging stuff in usr/src/pkgdefs, eg. ... >> usr/src/pkgdefs/Makefile >> usr/src/pkgdefs/SUNWgvim?/* > > Done. > >> usr/src/cmd/gvim/sunman-stability >> - will SUNW package name be SUNWvim or SUNWgvim? >> >> You need to add gvim into .. >> usr/src/cmd/Makefile > > Done. (Patch will be in next webrev). > >> You need to add usr/src/cmd/gvim/METADATA > > Not done because vim build does not do it. Our build implicitly > depends on vim (More on this below). > >> usr/src/cmd/gvim/Makefile.sfw >> Explicitly define 'configure --prefix=..'. >> Use the predefined value for '--prefix=..' from >> Makefile.master; or even better still use the stuff >> from CONFIGURE_OPTIONS, eg ... >> ./configure $(CONFIGURE_OPTIONS) >> see other recent integrations for examples, eg cups. > > Done. > >> You might want to apply 'Roland Mainz' comments ... >> - Use "env - ..." and not "env ..." in the Makefiles to >> make sure "configure" & "make" only see the env >> variables they should really get(and not pick-up >> any random env variable) >> - Use either $(SHELL) or /usr/bin/bash for "configure" >> calls (so we know which one is used and "configure" >> doesn't pick one itself). > > Done. > >> You could extract the version info from the METADATA >> file as per 'Christopher Mi' comment .. >> - Use the method define in Makefile.master >> since you have a standard METADATA file, eg. .. >> VER= $(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) >> TARBALL= $(VER).tar.bz2 > > Not done. Our build copies the files from vim71 dir (vim71 is > specified in the $VER) and copies it ot the gvim dir. It also copies > the vim-patches dir from vim to gvim. The idea is to build gvim > exactly as vim (including the same patches). Comments on this approach > awaited. > >> usr/src/cmd/gvim/install.sfw >> You might want to apply 'Roland Mainz' comments ... >> - use /usr/bin/ksh93 or /usr/bin/bash for install-* >> 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). > > These didn't work: > 1) "set -o errexit" fails with the error "-o option is not supported" > (It works in the terminal, but not in the build environment. Also it > wasn't in the vim makefile, which we are using as a template): > > (DESTDIR=/export/home/blah/sfwnv/proto/root_i386 /usr/bin/sh ./install.sfw) > ./install.sfw: -o: bad option(s) > *** Error code 1 > make: Fatal error: Command failed for target `install' > Current working directory /export/home/blah/sfwnv/usr/src/cmd/gvim > > 2) source ${SRC}/tools/install.subr fails with "source command not > found" (I find this strange this source is an internal bash command. > This also wasn't in the vim makefile). > > (DESTDIR=/export/home/blah/sfwnv/proto/root_i386 /usr/bin/sh ./install.sfw) > ./install.sfw: -o: bad option(s) > *** Error code 1 > make: Fatal error: Command failed for target `install' > Current working directory /export/home/blah/sfwnv/usr/src/cmd/gvim > 4:00 PM > > (DESTDIR=/export/home/blah/sfwnv/proto/root_i386 /usr/bin/sh ./install.sfw) > ./install.sfw: source: not found > removing binaries > doing sed magic stuff > ./install.sfw: _install: not found > mv: cannot access > //export/home/blah/sfwnv/proto/root_i386/usr/share/man/man1/gvim.1.temporary > ./install.sfw: _install: not found > mv: cannot access > //export/home/blah/sfwnv/proto/root_i386/usr/share/man/man1/gvimdiff.1.temporary > ./install.sfw: _install: not found > mv: cannot access > //export/home/blah/sfwnv/proto/root_i386/usr/share/man/man1/gvimtutor.1.temporary > >> As an example for all stuff required look at say ... >> http://cr.opensolaris.org/~jwalker/meld/ > > Great. > >> -- >> ---------------------------------------------------------------------- >> Paul Cunningham >> Software Engineer >> Tadpole Computer Products > > Thanks so much for your help. > > -Brian > > P.S. - Thanks for everyone for the comments. > > -- > - Brian Gupta > > http://opensolaris.org/os/project/nycosug/ > > http://www.genunix.org/wiki/index.php/OpenSolaris_New_User_FAQ > -- - Brian Gupta http://opensolaris.org/os/project/nycosug/ http://www.genunix.org/wiki/index.php/OpenSolaris_New_User_FAQ
