Brian, Just a few more nit-picking comments ...
Brian Gupta wrote: > New webrev posted: http://cr.opensolaris.org/~brandorr/gvim_webrev/ 1. usr/src/cmd/gvim/METADATA Why is Garrett listed as OWNER: and no you? You choose an older METADATA file as a template, later ones also have fields for ... SUPPORT: ???? BUGTRAQ: solaris/utility/??? 2. usr/src/cmd/gvim/Makefile.sfw Add line-space before 'clean:' In 'clean:' should you remove the stuff copied in line 84 (above it)? When using CONFIGURE_OPTIONS with configure its normally done as follows ... CONFIGURE_OPTIONS += --enable-gui=gtk2 CONFIGURE_OPTIONS += --with-vim-name=gvim CONFIGURE_OPTIONS += --enable-gtk2-check CONFIGURE_OPTIONS += --with-features=huge ... ... ... ./configure $(CONFIGURE_OPTIONS) I also just noticed you have CONIGURE_OPTIONS rather than CONFIGURE_OPTIONS. Line 50, do you need the DESTDIR=$(ROOT) before $(SHELL) ./install-sfw ? 3. usr/src/cmd/gvim/install-sfw You don't really need the 'echo' lines? I think the suggestion was that the script should use usr/bin/ksh93 or /usr/bin/bash rather than /bin/sh. 4. usr/src/pkgdefs/SUNWgvim/depend Cosmetic: on line 51, align the 'Vi IMproved' bit better with the stuff above it. 5. usr/src/pkgdefs/SUNWgvim/pkginfo.tmpl PKG="SUNWvim" should that be PKG="SUNWgvim" ? 6. usr/src/pkgdefs/SUNWgvim/prototype_com 49 f none usr/bin/gvim 0755 root bin does this need the write permission bit? Paul > > On Wed, Sep 24, 2008 at 6:14 AM, Paul Cunningham > <paul.cunningham at tadpole.com> wrote: >> >> A few more comments ... >> >> 1. usr/src/pkgdefs/SUNWgvim/depend >> Move Copyright lines to after "CDDL HEADER END" >> header > > Done. > >> 2. usr/src/cmd/gvim/install.sfw >> Cosmetic: this file is normally call install-sfw > > Done. > >> 3. usr/src/cmd/gvim/install.sfw >> You really should apply 'Roland Mainz' comments to this, >> you said it didn't work below but there are now a number >> of examples of it working in the gate. > > Paul, I broke up what you wrote earlier on this into subtasks: > > 3a) 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) > > Done. > > 3b) 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. > > 3c) You might want to apply 'Roland Mainz' comments ... > - use /usr/bin/ksh93 or /usr/bin/bash for install > > Done. > > 3d) add a $ set -o errexit # at the beginning: > > Done. We followed what was done in > http://cr.opensolaris.org/~jwalker/meld/raw_files/new/usr/src/cmd/meld/install-sfw > >> 4. usr/src/pkgdefs/SUNWgvim/Makefile >> Delete the null DATAFILES= line. > > Done. > >> 5. usr/src/pkgdefs/SUNWgvim/pkginfo.tmpl >> Copyright year is wrong > > Fixed. > >> 6. usr/src/pkgdefs/SUNWgvim/prototype_com >> & usr/src/pkgdefs/SUNWgvim/prototype_i386 >> & usr/src/pkgdefs/SUNWgvim/prototype_sparc >> Copyright line format is wrong and year. >> Package name on comment line is wrong. > > Fixed. > >> 7. usr/src/pkgdefs/SUNWgvim/pkginfo >> Delete this file as it's generated from pkginfo.tmpl > > Done. > >> 8. METADATA >> You have no usr/src/cmd/gvim/METADATA file in your >> webrev; if you haven't got one you need to create >> this. > > Added. > >> Paul >> >> Brian Gupta wrote: >>> 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 >>>> -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
