Roland Mainz wrote: > 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 "> "): > Not very clear here, are you talking the code of ltmain.sh ?. >> 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.) > > I will try later, but would like not to change it at this time. >> + "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 -- > > OK, Changed! > [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... > > OK. >> +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... > > OK >> + >> +. ${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) > > OK. >> + >> +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... > > OK. >> + >> +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) > ... > > OK. >> + 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... ;-( ) ? > > Yeah, it's not mine. :-) > [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 ? > > Again, I would like not to change it at this time. > >> + "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... > > OK. > [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 -- > > OK. > >> + >> +# 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... > > OK. > >> + >> +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... > OK. > The remaining bits look good for me... :-) > > Thanks very much for your time.
-- Henry Jiang -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20080526/e64395c9/attachment.html>
