Todd Pisek wrote:
> 
> Please see http://cr.opensolaris.org/~trp/ircii/webrev/ and provide any 
> comments or changes I need to make.

Quick race through
http://cr.opensolaris.org/~trp/ircii/webrev/sfw01.patch (patch code is
quoted with "> "):
> --- /dev/null   Fri Jun 27 09:30:49 2008
> +++ new/usr/src/cmd/ircii/Makefile.sfw  Fri Jun 27 09:30:49 2008
[snip]
> +
> +include ../Makefile.cmd
> +
> +VER=ircii-20060725

Please add these CFLAGS here:
CFLAGS += $(XPG6MODE) $(XSTRCONST)

> +
> +all: $(VER)/config.status
> +       (cd $(VER); env \

Please use "env -" to make sure the environment is cleared and only pass
environment variables which are really needed, e.g. that "gmake"+build
do not pick-up random env values from the build environment (which is
right now one of the top5 reasons why SFWNV may fail to build in some
cases).

> +           CC=$(CC) \
> +           "CFLAGS=$(CFLAGS)" \
> +           LD_OPTIONS="-M $(SRC)/cmd/mapfile_noexstk" \
> +           MAKE=$(CCSMAKE) \
> +           $(MAKE))
> +       @find . -name core -exec rm -f {} \;
> +
> +
> +install: all
> +       $(SH) ./install-sfw
> +
> +$(VER)/config.status: $(VER)/configure
> +       (cd $(VER); env \

Please use "env -" to make sure the environment is cleared and only pass
environment variables which are really needed, e.g. that "configure"
doesn't pick-up random env values from the build environment (which is
right now one of the top5 reasons why SFWNV may fail to build in some
cases).
[snip]

> +FRC:
> --- /dev/null   Fri Jun 27 09:30:50 2008
> +++ new/usr/src/cmd/ircii/install-sfw   Fri Jun 27 09:30:50 2008
> @@ -0,0 +1,109 @@
> +#! /usr/bin/sh -e

The "-e" is not used since the script is not executable and you call it
via $ sh install-sfw # - in which case this line is just a comment.

> +#
> +# CDDL HEADER START
> +#
> +# The contents of this file are subject to the terms of the
> +# Common Development and Distribution License {the "License"}.
> +# You may not use this file except in compliance with the License.
> +#
> +# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
> +# or http://www.opensolaris.org/os/licensing.
> +# See the License for the specific language governing permissions
> +# and limitations under the License.
> +#
> +# When distributing Covered Code, include this CDDL HEADER in each
> +# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
> +# If applicable, add the following below this CDDL HEADER, with the
> +# fields enclosed by brackets "[]" replaced with your own identifying
> +# information: Portions Copyright [yyyy] [name of copyright owner]
> +#
> +# CDDL HEADER END
> +#

Plese add "set -o errexit" and use either "ksh93" or "bash" to make sure
the script immediately stops if an error occurs.

[snip]
> +VERS=20060725
> +PKGVERS=ircii-${VERS}
> +TOPDIR=$PWD/${PKGVERS}
> +
> +. ${SRC}/tools/install.subr

Please use "source" instead of "." (see
http://www.opensolaris.org/os/project/shell/shellstyle/#use_source_not_dot).

AFAIK the remaining stuff looks ok...

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL <currently fluctuating>
 (;O/ \/ \O;)

Reply via email to