New webrev posted: http://cr.opensolaris.org/~brandorr/gvim_webrev/
Please see responses below.
-Brian
On Wed, Sep 24, 2008 at 6:14 AM, Paul Cunningham
<paul.cunningham at tadpole.com> wrote:
> Brian,
>
> 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
> General Dynamics Itronix Europe Ltd.
> Pioneer House
> Chivers Way
> Histon, Cambridgeshire,
> UK, CB24 9NL
> Ph: +44 (0)1223 200648
> FAX: +44 870 4324162
> Email: paul.cunningham at tadpole.com
>
> This email message is for the sole use of the intended
> recipient(s) and may contain GDC4S confidential or privileged
> information. Any unauthorized review, use, disclosure or
> distribution is prohibited. If you are not an intended
> recipient, please contact the sender by reply email and
> destroy all copies of the original message
>
--
- 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
