Paul: Thank you so much for doing this for me. Code review is always important and should be mandatory to help people like me to find some stupid mistakes ;)
Thank you again! Paul Cunningham wrote: > 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) I will change that follow the examples > > 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. I will check > > 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. Yes, this 'mkdir' is redundant > > 4. usr/src/pkgdefs/Makefile > You don't seem to have this in your webrev to include > SUNWautogen & SUNWguile My mistake:( > > 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. sorry about this stupid action > > 6. usr/src/pkgdefs/SUNWautogen/prototype_i386 > & usr/src/pkgdefs/SUNWautogen/prototype_sparc > pkg name is wrong in comment; SUNWguile ??? Another dummy error. > > 7. Everything else looks okay to me > > ==== End of Comments ====== >
