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
