Hi Paul,
Thanks ever so much for the review. I hope the powers that be appreciate that without you there'd be nothing in SFW, something we need to fix methinks. Changes at http://cr.opensolaris.org/~tekgrrl/FastCGI/ see inline for comments. Paul Cunningham wrote: > > 1. usr/src/lib/fastcgi/METADATA > You might want to include the blank COMMENTS: line ... > "http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines"; > so it conforms. Done > > 2. usr/src/lib/fastcgi/Makefile.sfw > You could replace PREFIX with CFGPREFIX from > Makefile.master > No harm in it so done. > You might want to do .. > Roland Mainz wrote: > > use either $(SHELL) or /usr/bin/bash for "configure" > > calls (so we know which one is used and "configure" > > doesn't pick one itself). This is really arbitrary, we should know what our configure scripts do. In the case of fastcgi, configure uses $CONFIG_SHELL to set the SHELL used by the Makefile (or /bin/sh if not set). I added CONFIG_SHELL=$(SHELL) to the vars used by configure and although it's not necessary I now invoke configure with $(SHELL). > > Combine lines ... > 139 -rm -rf $(VER) > 140 -rm -rf $(VER64) > 141 -rm -rf $(DESTDIR) > into one ... > -rm -rf $(VER) $(VER64) $(DESTDIR) Done > > Lines ... > 88 $(CCSMAKE) install > 94 $(CCSMAKE) install > why are these not installing into the ws proto area? Because of libtool. Believe me this is the only way to get libtool to behave correctly > > Why not use the same name for TMPDIR and DESTDIR > (its confusing) throughout ... (and into install-sfw*) Different things mus confuse different people, I found it confusing when I made them the same. You're the voice of reason though so I've changed it. > > Is this used ? ... > 76 LIBRARY=libfcgi.a Lint needs it. That's the limit of my lint knowledge :o) > > Could this lint stuff ... > 75 # lint stuff > 76 LIBRARY=libfcgi.a > 77 LINTOUT= lint.out > 78 LINTFLAGS= -I./include -I. $(CCBITS32) $(LARGEFILES) > 79 LINTFLAGS64= -I./include -I. $(CCBITS64) $(LARGEFILES) > be moved down to go with the lint rules? Sure. Done > > > 3. usr/src/lib/fastcgi/install-sfw > Do these need the write permission bit set ? ... > 117 _install N ../llib-lfcgi ${LIBDIR}/llib-lfcgi 0644 > 118 _install N llib-lfcgi.ln ${LIBDIR}/llib-lfcgi.ln 0644 ) > > and in .. > usr/src/lib/fastcgi/install-sfw-64 > > and in .. > usr/src/pkgdefs/SUNWfcgi/prototype_com > usr/src/pkgdefs/SUNWfcgi/prototype_i386 > usr/src/pkgdefs/SUNWfcgi/prototype_sparc \ Nope, changed in all files. > > 4. pkg copyright files (SUNW*/copyright) > You probably need to add the source owner copyright lines > at the top at the top of this, ie. those extracted from > the src code files in the unpacked tarballs. I've done this but if I'm going to ask others to do the same I'd really like to know where this instruction came from, is it documented somewhere? It does kind of make sense, but then I'm not a lawyer. > > 5. usr/src/pkgdefs/SUNWfcgi-doc/depend > Does the doc package really depend on SUNWfcgi, ie. > to install and look at the doc do I have to have > SUNWfcgi installed also? I'm guess, no. I guess not, to me it seemed logical though. I've changed it , you can now install the docs separately > > 6. SUNWfcgi-doc > Is there a reason why the doc has been split out > from the main package? > Yes, because there's lots of it, all of it is online and there's no point in having it if you don't need it. For that reason I specified separate packages in the ARC case.
