Norm Jacobs wrote:
> I figured that I might not get a chance to look at this later this
> weekend and
> decided to pass on a few hand of poker so I took a look at your
> webrev. I
> didn't go through this in the detail I might have liked, but it's
> getting late.
> Here are some comments.
>
Hi Norm,
Sorry for late response. Actually I have tried a lot from the very
beginning, especially for 64bit, but it didn't work as expected, so you
see the many tricky stuffs.
Thanks for your review!
please see my replies below.
> -Norm
>
> usr/src/lib/ilmbase/Makefile.sfw
> Line 34:
> "all" target should be building the acutal bits and perhaps
> split into
> 32/64 bit targets.
> (cd $(VERS) ; $(CCSMAKE))
> (cd $(VERS)-64 ; $(CCSMAKE))
OK.
> Line 41:
> If you plan on using the component's install target, you might use
> INSTALL="$(INSTALL_PROTO)", which guarantee files install
> in the
> proto area and will post process them as they are
> installed. On the
> otherhand, if you use an install-xxx script, it doesn't
> matter.
This is new to me. I should have used it at start.
> Line 42:
> You should quote the values in setting the environment variables
> CC="$(CC)" CXX="$(CCC)"
OK.
> Lines 43:
> You shouldn't need to specify these libraries, they should be
> in the
> default set when using CC to link C++ programs and libraries
These were intentionally added, Otherwise it will fail.
> Line 44:
> Presumably you will want to set --prefix=$(CFGREFIX) and
This is OK.
> --mandir=$(CFGMAN)
No man pages in the source package.
> Lines 45:
> You don't need to "cd .." in your subshell, leaving the
> subshell () puts
> you right back where you started. Also, this doens't belong
> here. It
> belongs as part of the "all" target.
The install need run immediately after compiling.
> Line 46:
> You should use $(SH), $(KSH93) or whatever shell this was
> written for,
> but not $(SHELL). Also this doesn't belong here.
Actually SHELL is defined as "ksh93" while "KSH93" is undefined:
# grep -i ksh93 usr/src/Makefile*
usr/src/Makefile.master:# this can help catch problems if sh is ksh93
usr/src/Makefile.master:SHELL=/usr/bin/ksh93
usr/src/Makefile.master:%: %.ksh93
> It belongs as part
> the "all" target
> Line 48: see line 41
> Line 49: see line 42, but use the 64-bit variants.
> CC="$(CC64)" CXX="$(CCC64)"
OK.
> Line 50: use the predefined macros from Makefile.master, ...
> CXXFLAGS="$(CCFLAGS64)"
The build will fail, for some options of "CCFLAGS64" was not acceptable.
> Lines 51-54: see lines 43-46
> Lines 56-76:
> You shouldn't need any of these patches if you define
> CXXLINK="$(CCC) -norunpath"
> in the calling environment to configure.
> CXXLINK="$(CCC64) -norunpath"
I have already tried this before, but still got these stuff, is it
acceptable?
# elfdump -d libImath.so.6.0.0 | grep PATH
[9] RUNPATH 0xf82
/opt/SUNWspro/SS11/lib/rw7/amd64:/opt/SUNWspro/SS11/lib/amd64:/usr/ccs/lib/64:/lib/64:/usr/lib/64
[10] RPATH 0xf82
/opt/SUNWspro/SS11/lib/rw7/amd64:/opt/SUNWspro/SS11/lib/amd64:/usr/ccs/lib/64:/lib/64:/usr/lib/64
> for 64-bit
> usr/src/lib/ilmbase/install-ilmbase
> usr/src/lib/ilmbase/install-ilmbase-64
> pulling bits out of .libs directories can be dangerous because
> libtool has
> a final relink that it does prior to installation. You might
> seriously
> consider just using the (cd $(VER) ; make install) approach to
> installation
> instead of ($(SH) ./install-XXX).
> For the 64 bit support you may have to do something like
> (cd $(VER)-64 ; make DESTDIR=$(SRC)/.../$(VER)-64/proto install
> ; \
> $(SH) ../install-XXX-64)
> to get the bits linked properly and pick out what you want to put
> in the
> proto area.
OK, I will retry this.
> I notice that you are installing writable files in /usr, don't
> There is no
> reason for them to be 644 or 755 when 444 and 555 is sufficient.
> If you "make install", you will want to use proto-fix to make sure
> that the
> permissions match the packaging in your makefile.sfw
> $(SRC)/tools/protofix --pkg $$pkg --perm
>
OK.
> usr/src/lib/ilmbase/ltmain.sh
> Why the new ltmain?
Strangely enough, the old ltmain (libtool) will throw out 64bit options.
> usr/src/lib/ilmbase/patch/Makefile.in.Half.patch
> usr/src/lib/ilmbase/patch/Makefile.in.Half64.patch
> usr/src/lib/ilmbase/patch/Makefile.in.Iex.patch
> usr/src/lib/ilmbase/patch/Makefile.in.Iex64.patch
> usr/src/lib/ilmbase/patch/Makefile.in.IlmThread.patch
> usr/src/lib/ilmbase/patch/Makefile.in.IlmThread64.patch
> usr/src/lib/ilmbase/patch/Makefile.in.Imath.patch
> usr/src/lib/ilmbase/patch/Makefile.in.Imath64.patch You shouldn't
> need any of these patches.
>
In my patch, I use cc instead of CC to link the libraries, which can
keep RUNPATH clean. The binraries linked by CC, contained these paths.
# elfdump -d libImath.so.6.0.0 | grep PATH
[9] RUNPATH 0xf82
/opt/SUNWspro/SS11/lib/rw7/:/opt/SUNWspro/SS11/lib/:/usr/ccs/lib/:/lib/:/usr/lib/
[10] RPATH 0xf82
/opt/SUNWspro/SS11/lib/rw7/:/opt/SUNWspro/SS11/lib/:/usr/ccs/lib/:/lib/:/usr/lib/
I don't know the difference, but it works.
> usr/src/lib/openexr/Makefile.sfw
> I have similar comments to the ilmbase Makefile.sfw and
> at lines 51 and 57, --prefix=$(CFGPREFIX), not =$(ROOT)/usr
openexr will need header files and libraries from $(ROOT)/usr, which
were installed by ilmbase.
# grep CFGPREFIX usr/src/Makefile.*
usr/src/Makefile.master:CFGPREFIX= /usr
>
> usr/src/lib/openexr/install-openexr
> usr/src/lib/openexr/install-openexr-64
> see corresponding ilmbase comment
> usr/src/lib/openexr/ltmain.sh.new
> see corresponting ilmbase comment
>
> usr/src/lib/openexr/patch/Makefile.in.IlmImf.patch
> usr/src/lib/openexr/patch/Makefile.in.IlmImf64.patch
> usr/src/lib/openexr/patch/Makefile.in.exrenvmap.patch
> usr/src/lib/openexr/patch/Makefile.in.exrheader.patch
> usr/src/lib/openexr/patch/Makefile.in.exrmakepreview.patch
> usr/src/lib/openexr/patch/Makefile.in.exrmaketiled.patch
> usr/src/lib/openexr/patch/Makefile.in.exrstdattr.patch
> You shouldn't need these patches.
>
> usr/src/pkgdefs/SUNW*/prototype_*
> Lots of stuff that doesn't need to be writable.
>
OK.
I will update webrev later this week.
Thanks again!
--
Henry Jiang
