Steve, See below for my comments ...
Paul steve xue wrote: > Please kindly do code review for my pkg porting SUNWguile and SUNWautogen: > > http://cr.opensolaris.org/~xdstone1/webrev-guile-and-autogen/ > > > More info about GNU autogen and guile are here: > http://www.gnu.org/software/guile/ > http://www.gnu.org/software/autogen/ ==== Start of Comments ==== 1. usr/src/cmd/autogen/Makefile.sfw & usr/src/cmd/guile/Makefile.sfw You might want to consider using the predefined value for '--prefix=' from Makefile.master, see the following as an example http://cr.opensolaris.org/~rayx/erlang/webrev/usr/src/cmd/erlang/Makefile.sfw.html You might want to consider using compiler flag ... > Roland Mainz wrote: > the "-xstrconst" puts all string literals into > read-only memory (e.g. it's shared between processes and > won't waste memory then). but ... > Keith M Wesolowski wrote: > The right way to set these flags, if in fact they are > appropriate for the source in question, is: > CFLAGS += $(XSTRCONST) 2. usr/src/cmd/autogen/install-sfw Someone else pointer out that defining 'MANSCRIPT=../sunman-stability' is not required anymore. Check and if not remove. 3. usr/src/cmd/guile/install-sfw I don't think you need 'mkdir -p ...' as the directory should already exist as it's in Targetdirs. 4. usr/src/pkgdefs/Makefile You don't seem to have this in your webrev to include SUNWautogen & SUNWguile 5. usr/src/pkgdefs/SUNWautogen/pkginfo & usr/src/pkgdefs/SUNWguile/pkginfo This shouldn't be in the webrev as it's really the pkginfo.tmpl file modified by build system. 6. usr/src/pkgdefs/SUNWautogen/prototype_i386 & usr/src/pkgdefs/SUNWautogen/prototype_sparc pkg name is wrong in comment; SUNWguile ??? 7. Everything else looks okay to me ==== End of Comments ====== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
