Hi Paul! Thank you for your review (yet again! :-)
My replies inline. Paul Cunningham wrote: > 1. The new and old tarball do not show up in the webrev, > to indicated what is happening to them. > > 2. usr/src/lib/lcms/Makefile.sfw > Could the patches (4 of them) ... > gpatch -p0 <../libtool.1.patch > be done with the other patches at the bottom with the tarball > uncompresses? Well ... not really. :-) Because libtool is generated by ./configure. I'm patching the ./configure - generated libtool. All the other patches are applied before ./configure is run. The actual contents of libtool (and the stuff i am patching inside libtool) are generated by a combination of a very large chunk of ./configure + ./aclocal.m4 + /usr/share/aclocal/libtool.m4. I have to do this because libtool (in its infinite wisdom) decides to replace the C++ Studio flags as written in the Makefile with some on-the-fly generated stuff. > Lines .. > 83 all: real-all > 274 real-all: all32 all64 lint32 lint64 > could that just be .. > all: all32 all64 lint32 lint64 Yup, you're right. :-) > > Line ... > 35 VER=lcms-$(RELNUM) > could you extract the lcms from the METADATA NAME field ? Yes. > > Line ... > 276 test: test32 test64 > do you need this, as there are no test32 test64 rules ? Will add test rules. There is a test harness for ncat only. > Line ... > 41 PREFIX = /usr > use the CFGPREFIX defined in Makefile.master rather than > hard coding /usr Yes. > > Lines ... > 52 PYTHON24 = /usr/bin/python2.4 > 53 PYTHON26 = /usr/bin/python2.6 > could you use the ones defined in Makefile.master Yes. > > Lines ... > 54 PYTHON_PATH_24 = /usr/lib/pytyhon2.4/vendor-packages > 55 PYTHON_PATH_24_64 = /usr/lib/pytyhon2.4/vendor-packages/64 > 56 PYTHON_PATH_26 = /usr/lib/pytyhon2.6/vendor-packages > 57 PYTHON_PATH_26_64 = /usr/lib/pytyhon2.6/vendor-packages/64 > is that a a typo ? pytyhon ? Yes that is a typo. > > Lines (4 if them) ... > "./configure ..... \ > ..... \ > etc ..." > could you use the CONFIGURE_OPTIONS method of setting the > options rather than duplicating them 4 times, eg .. > > CONFIGURE_OPTIONS += --localstatedir=/var > CONFIGURE_OPTIONS += --enable-shared > etc > ..... > ..... > ./configure $(CONFIGURE_OPTIONS) > ..... > ..... > ./configure $(CONFIGURE_OPTIONS) $(CONFIGURE_OPTIONS64) Yes. > > Lines ... > 390 cd ..) > 400 > 410 > 420 > does it really need these lines? It doesn't need the cd .. but it doesn't hurt either. > > Lint stuff .. > is this required ? Yes. > > 3. usr/src/pkgdefs/SUNWlcms/depend > The copyright lines should probably be moved to after the > "CDDL HEADER END" header > > 4. usr/src/lib/lcms/METADATA > Why the "==========..." line (line 14)? Something about the METADATA parser which wants that line to figure out where the free-form comments start (if there are any free-form comments in METADATA). I'll fix these things -- thank you very much for the review! -Stefan -- Stefan Teleman Oracle Corporation stefan.teleman at Sun.COM
