Once more into the breech dear friends... http://cr.opensolaris.org/~brandorr/gvim_webrev/
On Fri, Sep 26, 2008 at 5:04 AM, Paul Cunningham <paul.cunningham at tadpole.com> wrote: > 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? He is the sponsor for this ARC case. I assumed that was who needed to be listed. Can people please verify? (I can change this if necessary). > You choose an older METADATA file as a template, later > ones also have fields for ... > SUPPORT: ???? > BUGTRAQ: solaris/utility/??? Added. (Based on SUNWMercurial) > 2. usr/src/cmd/gvim/Makefile.sfw > Add line-space before 'clean:' Done. > In 'clean:' should you remove the stuff copied > in line 84 (above it)? Done. > 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) Done. > I also just noticed you have CONIGURE_OPTIONS rather > than CONFIGURE_OPTIONS. That's the "New American" spelling. Seriously though, this was a typo; fixed > 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? Was added for debugging purposes. Removed. > I think the suggestion was that the script should use > usr/bin/ksh93 or /usr/bin/bash rather than /bin/sh. Changed/fixed. > 4. usr/src/pkgdefs/SUNWgvim/depend > Cosmetic: on line 51, align the 'Vi IMproved' bit > better with the stuff above it. Done. > 5. usr/src/pkgdefs/SUNWgvim/pkginfo.tmpl > PKG="SUNWvim" should that be PKG="SUNWgvim" ? Fixed. > 6. usr/src/pkgdefs/SUNWgvim/prototype_com > 49 f none usr/bin/gvim 0755 root bin > does this need the write permission bit? This is the way the "vim" executable is packaged in SUNWvim. I can't see any reason why we can't change this to 0555, but for this webrev I've kept it to 0755. Cheers, Brian > 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 > -- - 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
