Hi Pavel,
Why is mrxvt being configured with bare minimum features (configure)
options ?? . It would be good to add support for
--enable-menubar , --enable-xft, --enable-24bits and other features.
~ Pradhap.D
Paul Cunningham wrote:
> Pavel,
>
> See below for my comments ...
>
> Paul
>
> Pavel Tatashin wrote:
>
>> I would like to ask you to review mrxvt port:
>>
>> http://cr.opensolaris.org/~soleen/webrevs/mrxvt_01/
>>
>> No changes to mrxvt package at all, I added argument "-xc99=all,no_lib"
>> because the default -xc99=all for some reason makes mrxvt not to
>> update tput cols/tput lines values.
>> I also had to manually convert manual page, because it was in -mdoc format.
>>
>
> === Start of Comments ====
>
> 1. usr/src/cmd/mrxvt/METADATA
> Add lines for ..
> URL: <link to source code location>
> SUPPORT: ????
> BUGTRAQ: solaris/utility/????
>
> 2. usr/src/cmd/mrxvt/Makefile.sfw
> 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.
>
> 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).
>
> 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
>
> 3. usr/src/cmd/mrxvt/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).
>
> I don't think you need the '_install M ..' for your
> man page is it already includes the sun stability stuff;
> so you can just use '_install N ..' I think (but check).
>
> 4. usr/src/cmd/mrxvt/mrxvt.1.sunman
> Does this need a sun copyright statement in it?
>
> 5. usr/src/pkgdefs/SUNWmrxvt/depend
> This looks like the default 'depend' file; if you have
> no other dependencies for this package then ...
> - remove this file
> - add 'DATAFILES= depend' to SUNWmrxvt/Makefile
>
> But have you checked the dependencies using the
> dependency checker script(s) to ensure you have
> no others. I think Alan Coopersmith could probably
> point you too those scripts.
>
> If you keep this file, move the Copyright lines to
> after the 'CDDL HEADER END'.
>
> 6. usr/src/pkgdefs/SUNWmrxvt/pkginfo.tmpl
> You don't need the version on the NAME= line.
>
> 7. usr/src/pkgdefs/SUNWmrxvt/prototype_com
> & usr/src/cmd/mrxvt/install-sfw
> Do all these files (f) need the 'write' permission
> bit set? Switch it off if not in /usr.
>
> 8. usr/src/pkgdefs/SUNWmrxvt/prototype_sparc
> & usr/src/pkgdefs/SUNWmrxvt/prototype_i386
> Not that is matters (being picky), but these normally
> have the pkg name on a comment line at the bottom.
>
> === End of Comments ======
>
>
