Brian,
A few more comments ...
1. usr/src/pkgdefs/SUNWgvim/depend
Move Copyright lines to after "CDDL HEADER END"
header
2. usr/src/cmd/gvim/install.sfw
Cosmetic: this file is normally call install-sfw
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.
4. usr/src/pkgdefs/SUNWgvim/Makefile
Delete the null DATAFILES= line.
5. usr/src/pkgdefs/SUNWgvim/pkginfo.tmpl
Copyright year is wrong
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.
7. usr/src/pkgdefs/SUNWgvim/pkginfo
Delete this file as it's generated from pkginfo.tmpl
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.
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
