Dermot McCluskey wrote:
> I've implemented all your suggested changes except the first one (CC 
> options).  If its building
> and running fine for me now, what improvement would the CC changes make for 
> me?  I
> notice that some other SFW apps are building with -xc99=%all (or 
> -xc99=%none), but they
> might need that to build with Sun Studio?  I don't see any other projects 
> building with those
> -D options.

See ksh93-integration (which integrated into OS/Net) for a precedent.
The main reason was to get more performance, shrink libast and better
standards conformance (since we aim at replacing /usr/xpg4/bin/sh (the
standard POSIX shell)). For your case it may be recommended since system
calls like |popen()|, |system()| would use /usr/xpg4/bin/sh (POSIX
shell) instead of /sbin/sh (old Bourne shell (not "bash")) which is
closer to what today's applications and users expect for a default
system shell (see indiana-discuss at opensolaris.org for ongoing flamewars
about shells (the really silly part is... we could simply burry this
mess by making the C99+XPG6-mode the default for SFW and most rants
about shells and "libc being ancient, let's replace it with glibc" etc.
would likely cease... ;-/ ) ... ;-( )

> Also, I've applied your suggestions to Tk as well as Tcl, and to install-sfw 
> as well as
> install-sfw-64. Updated webrev:
> http://cr.opensolaris.org/~dermot/tcltk64-02/

Quick look (patch code is quoted with "> "):
- new/usr/src/lib/tcl/Makefile.sfw
> +SHELL=/bin/ksh
> +

Ewww (no, don't worry, I am not complaining about /bin/ksh vs.
/bin/ksh93 :-) ) ...
... nitpicking: Please change this to SHELL=/usr/bin/ksh (since /bin is
a softlink to /usr/bin/)

> +test64:
> +     (cd $(VER64)/unix; env \
> +         CC=$(CC64) "CFLAGS=$(CFLAGS64) $(XREGSFLAG64) -xstrconst" \
> +         MAKE=/usr/ccs/bin/make \
> +         $(MAKE) test)
> +     @find . -name core -exec rm -f {} \;
> +

Just curious: Why is "env" used in this case (I can't imagine a reason
since $ FOO=bar application # and $ env FOO=bar application # are more
or less identical in this situation) ?

> -         for i in *.n ; do manbase=`basename $$i .n`; mv $$i $$manbase.1t; 
> done ; \
> -         for i in *.3 ; do manbase=`basename $$i .3`; mv $$i $$manbase.3tcl; 
> done)
> +         for i in *.n ; do manbase="$$(basename "$$i" .n)"; mv "$$i" 
> "$$manbase.1t"; done ; \
> +         for i in *.3 ; do manbase="$$(basename "$$i" .3)"; mv "$$i" 
> "$$manbase.3tcl"; done)

Nitpicking: '{'+'}' around "manbase" may be nice if it's followed by a
'.' (to avoid user confusion when shells (like ksh93) allow things like
compound variables (e.g. $ ksh93 -c 'x=( y="hello") ; echo "${x.y}"'
#)).

> +$(VER64)/unix/configure: $(VER)-src.tar.gz
> +     mkdir -p tmp
> +     gzip -dc $(VER)-src.tar.gz | (cd tmp; tar xopf -)
> +     rm -rf $(VER64)
> +     mv tmp/$(VER) $(VER64); rmdir tmp

Nitpicking: You can bind these things together via "; \" at the end of
each line (otherwise "make" will execute a seperate shell for each
single command and acts a bit like a low-yield |fork()|-bomb) until the
next "major" application/tool/etc. runs (that's "gzip" AFAIK).


- usr/src/lib/tcl/install-sfw (similar issues may apply to the 64bit
sibling of the script):

Generic issue: Is the script really only called from with the Makefile ?
If not (or in the case you want to be on the "safe" side please set PATH
to a fixed value (where "set" really means set and not something like
PATH="${PATH}:/bin/" (which may cause trouble if the user's PATH may
contain something unexpected...)).

> --- old/usr/src/lib/tcl/install-sfw   Fri Nov  2 05:08:16 2007
> +++ new/usr/src/lib/tcl/install-sfw   Fri Nov  2 05:08:16 2007
> @@ -1,4 +1,4 @@
> -#!/bin/sh
> +#!/bin/ksh

s/\/bin\/ksh/usr\/bin\/ksh/

>   35 . ${SRC}/tools/install.subr

Please use "source" instead of "." ("source" is identical except that
it's more readable and that you can check for errors - see
http://www.opensolaris.org/os/project/shell/shellstyle/#use_source_not_dot)
/same issue applies to the 64bit sibling of the script).

Nit: Quotes around variable expansions are missing (e.g. $ mv
"/a/${foo}" /a/x # instead of  $ mv /a/${foo} /a/x #) ... (not mandatory
if you are sure that the matching variables don't contain garbage by
accident (which may let the script run&&jump amok...))


- usr/src/lib/tk/Makefile.sfw

>  clean:
>       -rm -rf $(VER)
> +     -rm -rf $(VER64)

Nit:
-- snip --
-rm -rf \
    $(VER) \
    $(VER64)
-- snip --
... one saved |fork()|+|exec()|

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 7950090
 (;O/ \/ \O;)

Reply via email to