Sheng-qi Jiang wrote: > > Please help review the inclusion of openexr-1.6.1 and ilmbase-1.0.1 into > the sfw consolidation. > http://cr.opensolaris.org/~henryj/openexr/
5min race through http://cr.opensolaris.org/~henryj/openexr/sfwexr.patch (patch code is quoted with "> "): > BASE=ilmbase-1.0.1 > +ILMBASE64=ilmbase-1.0.1-64 > + > +include ../Makefile.lib > +include ../Makefile.lib.64 > + > +all: $(ILMBASE)/config.status > + > +install: all > + $(SH) ./install-ilmbase > + MACH64=$(MACH64) $(SH) ./install-ilmbase-64 IMO it may be nice to set: -- snip -- -xc99=%all -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1 -xstrconst -xspace -- snip -- ... as default CFLAGS/CXXFLAGS - "-xc99=%all -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1" enables C99/XPG6 conformance mode, "-xstrconst" folds duplicate constant strings into one (saves runtime memory, like gcc), "-xspace" tries to save space in code. If you have any problems with these flags contact me offline... > +$(ILMBASE)/config.status: $(ILMBASE)/configure > + (cd $(ILMBASE); env "INSTALL=/usr/ucb/install -c" \ > + CC=$(CC) CXX=$(CCC) \ > + "LDFLAGS=-lCstd -lCrun -lm -lc" \ Why aren't you passing CFLAGS and CXXFLAGs through in this case ? > + ./configure --prefix=$(ROOT)/usr ) > + (cd $(ILMBASE); $(CCSMAKE) -e; cd ..; ) > + ($(SH) ./install-ilmbase) IMO it may be nice to use /usr/bin/ksh93 -o errexit ./install-ilmbase # to make sure the script exits on the first error instead of continuing (see below) ... > + (cd $(ILMBASE64); env "INSTALL=/usr/ucb/install -c" \ > + CC=$(CC) CXX=$(CCC) \ > + "CXXFLAGS=-xO3 -xarch=generic64 -Ui386 -U__i386 -compat=5" \ See above (C99/XPG6 mode, -xstrconst etc.) > + "LDFLAGS=-lCstd -lCrun -lm -lc" \ > + ./configure --prefix=$(ROOT)/usr; cd .. ) > + (cd $(ILMBASE64); $(CCSMAKE) -e; cd .. ) > + (MACH64=$(MACH64) $(SH) ./install-ilmbase-64) > + > +$(ILMBASE)/configure: $(ILMBASE).tar.gz > + gzip -dc $(ILMBASE).tar.gz | tar xopf - > + touch $(ILMBASE)/configure > + mv $(ILMBASE) $(ILMBASE64) > + cp -p ltmain.sh.new $(ILMBASE64)/ltmain.sh > + gzip -dc $(ILMBASE).tar.gz | tar xopf - > + touch $(ILMBASE)/configure > + > +clean: > + -rm -rf $(ILMBASE) > + -rm -rf $(ILMBASE64) Please combine these "rm" calls, e.g. -- snip -- -rm -rf \ $(ILMBASE) \ $(ILMBASE64) -- snip -- [snip] > +++ new/usr/src/lib/ilmbase/install-ilmbase-64 Fri May 23 17:35:39 2008 > @@ -0,0 +1,56 @@ > +#!/bin/sh [snip] Please use a shell (e.g. ksh93, bash) which understands "set -o errexit" ("exit script (or function scope) at the first error") and use it here to abort the script when an error occurs. The idea is to prevent the script from running amok if something goes wrong... > +VERS64=ilmbase-1.0.1-64 > + > +PREFIX=${ROOT}/usr > +LIBDIR=${PREFIX}/lib/${MACH64} > +LIBsDIR=${PREFIX}/lib/.libs > +INCDIR=${PREFIX}/include/OpenEXR > +SHAREDIR=${PREFIX}/share > +MAN3DIR=${SHAREDIR}/man/man3 It may be nice to use double-quotes... > + > +. ${SRC}/tools/install.subr Please use.. -- snip -- source "${SRC}/tools/install.subr" -- snip -- (see http://www.opensolaris.org/os/project/shell/shellstyle/#use_source_not_dot) > + > +cd ${VERS64} > + > +_install D Half/.libs/libHalf.so.6.0.0 ${LIBDIR}/libHalf.so.6.0.0 755 > +_install L libHalf.so.6.0.0 ${LIBDIR}/libHalf.so.6 > +_install L libHalf.so.6 ${LIBDIR}/libHalf.so Please replace this with... -- snip -- _install D "Half/.libs/libHalf.so.6.0.0" "${LIBDIR}/libHalf.so.6.0.0" 755 _install L "libHalf.so.6.0.0" "${LIBDIR}/libHalf.so.6" _install L "libHalf.so.6" "${LIBDIR}/libHalf.so" -- snip -- ... e.g. add double-quotes to prevent the shell from doing any globbing or other pattern expension... > + > +for subd in "Iex" "Imath" "IlmThread" > +do > + cd ${subd} > + _install D .libs/lib${subd}.so.6.0.0 ${LIBDIR}/lib${subd}.so.6.0.0 755 > + _install L lib${subd}.so.6.0.0 ${LIBDIR}/lib${subd}.so.6 > + _install L lib${subd}.so.6 ${LIBDIR}/lib${subd}.so Same as above, please use double-quotes (see http://www.opensolaris.org/os/project/shell/shellstyle/#quote_variables_containing_filenames_or_userinput) ... > + cd .. > +done > + > +exit 0 [snip] > --- /dev/null Fri May 23 17:35:41 2008 > +++ new/usr/src/lib/ilmbase/ltmain.sh.new Fri May 23 17:35:40 2008 > @@ -0,0 +1,6930 @@ This script comes from upstream, right (otherwise I need two hours to write a _long_ email about it... ;-( ) ? [snip] > --- /dev/null Fri May 23 17:35:43 2008 > +++ new/usr/src/lib/openexr/Makefile.sfw Fri May 23 17:35:42 2008 > @@ -0,0 +1,75 @@ [snip] > +EXR=openexr-1.6.1 > +EXR64=openexr-1.6.1-64 > + > +include ../Makefile.lib > +include ../Makefile.lib.64 Same as above - please use "CFLAGS"/"CXXFLAGS" set to "-xc99=%all -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1 -xstrconst -xspace" ... > +all: $(EXR)/config.status > + (cd $(EXR); \ > + env LD_LIBRARY_PATH=$(ROOT)/usr/lib:/usr/lib:/usr/sfw/lib \ > + $(CCSMAKE) -e) > + > + (cd $(EXR64); \ > + env LD_LIBRARY_PATH=$(ROOT)/usr/lib/$(MACH64):/usr/lib:/usr/sfw/lib \ > + $(CCSMAKE) -e) > + > +install: all > + $(SH) ./install-openexr > + MACH64=$(MACH64) $(SH) ./install-openexr-64 Same as above - please use a shell which understands "set -o errexit" (ksh93, bash) and use this flag here... > +$(EXR)/config.status: $(EXR)/configure > + (cd $(EXR); env "INSTALL=/usr/ucb/install -c" \ > + CC=$(CC) CXX=$(CCC) \ Why aren't CFLAGS/CXXFLAGS not passed-thought ? > + "LDFLAGS=-lCstd -lCrun -lm -lc" \ > + ./configure --disable-ilmbasetest --prefix=$(ROOT)/usr ) > + > + (cd $(EXR64); env "INSTALL=/usr/ucb/install -c" \ > + CC=$(CC) CXX=$(CCC) \ > + "CXXFLAGS=-xO3 -xarch=generic64 -Ui386 -U__i386 -compat=5" \ Why aren't CFLAGS/CXXFLAGS not set in the Makefile above and then passed-thought in this statement ? > + "LDFLAGS=-lCstd -lCrun -lm -lc" \ > + ./configure --disable-ilmbasetest --prefix=$(ROOT)/usr ) > + > +$(EXR)/configure: $(EXR).tar.gz > + gzip -dc $(EXR).tar.gz | tar xopf - > + touch $(EXR)/configure > + mv $(EXR) $(EXR64) > + cp -p ltmain.sh.new $(EXR64)/ltmain.sh > + gzip -dc $(EXR).tar.gz | tar xopf - > + touch $(EXR)/configure > + > +clean: > + -rm -rf $(EXR) > + -rm -rf $(EXR64) Same as above, please combine these "rm" calls into one... [snip] > > --- /dev/null Fri May 23 17:35:47 2008 > +++ new/usr/src/lib/openexr/install-openexr Fri May 23 17:35:46 2008 > @@ -0,0 +1,98 @@ > +#!/bin/sh [snip] > + > +VERS=openexr-1.6.1 > + > +PREFIX=${ROOT}/usr > +LIBDIR=${PREFIX}/lib > +BINDIR=${PREFIX}/bin > +INCDIR=${PREFIX}/include/OpenEXR > +SHAREDIR=${PREFIX}/share > +MAN1DIR=${SHAREDIR}/man/man1 > +MAN3DIR=${SHAREDIR}/man/man3 > +EXAMPLES=${SHAREDIR}/doc/${VERS}/examples As said above, plese add "set -o errexit" here... > + > +MANSCRIPT=./sunman-stability > + > +. ${SRC}/tools/install.subr Please use... -- snip -- source "${SRC}/tools/install.subr" -- snip -- > + > +# install some stuff that doesn't come out of the source distribution > +# (i.e. sun-specific stuff) from the base directory before installing > +# from the build directory > + > +# manpages are special "sun" versions with corrected section > +# references, etc. These are the ones we actually install. > +# Note to maintainers - if the package revs, you need to re-create > +# new sun versions of the manpages. > + > +_install M libopenexr.3lib ${MAN3DIR}/libopenexr.3lib 444 > +_install M exrenvmap.1 ${MAN1DIR}/exrenvmap.1 444 > +_install M exrheader.1 ${MAN1DIR}/exrheader.1 444 > +_install M exrmakepreview.1 ${MAN1DIR}/exrmakepreview.1 444 > +_install M exrmaketiled.1 ${MAN1DIR}/exrmaketiled.1 444 > +_install M exrstdattr.1 ${MAN1DIR}/exrstdattr.1 444 Please add double-quotes around the 4rd argument... > + > +cd ${VERS} > +#_install N OpenEXR.pc ${LIBDIR}/pkgconfig/OpenEXR.pc 644 > + > +cd IlmImf > +_install D .libs/libIlmImf.so.6.0.0 ${LIBDIR}/libIlmImf.so.6.0.0 755 > +_install L libIlmImf.so.6.0.0 ${LIBDIR}/libIlmImf.so.6 > +_install L libIlmImf.so.6 ${LIBDIR}/libIlmImf.so Doube-quotes... > + > +for i in Imf*.h > +do > + _install N ${i} ${INCDIR}/${i} 644 Doube-quotes... > +done > + > +cd .. > +_install N config/OpenEXRConfig.h ${INCDIR}/OpenEXRConfig.h 644 Doube-quotes... > + > +for i in "exrheader" "exrmaketiled" "exrstdattr" "exrmakepreview" "exrenvmap" > +do > + cd ${i} > + _install N .libs/${i} ${BINDIR}/${i} 755 Doube-quotes... > + cd .. > +done > + > +cd IlmImfExamples > +for i in * > +do > + _install N ${i} ${EXAMPLES}/${i} 644 Doube-quotes... > +done > +cd .. > + > +cd doc > +for i in *.pdf > +do > + _install N ${i} ${SHAREDIR}/doc/${VERS}/${i} 644 Doube-quotes... The remaining bits look good for me... :-) ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;)
