Paul, Thanks for your careful code review. Really appreciate it. My comments below:
Regards, Faramarz Jalalian Paul Cunningham wrote: > Faramarz, > > See below for my comments, some of them may have duplicated from other > review .... > > Paul > > Faramarz Jalalian wrote: >> >> Would you please review the changes to include CMake into >> OpenSolaris? You can find the webrev at: >> >> http://cr.opensolaris.org/~jalalian/cmake/ >> >> There is "almost" no changes made to the cmake. To make installation >> easier, the configure script is passed >> /usr as build prefix, but cmake install target is modified to install >> everything under sfw_stage directory so that >> later install-sfw can pick them up and install them in the right place. > > === Start of Comments ==== > > 1. usr/src/cmd/cmake/Makefile.sfw > Why have you put this in ? ... > "# Portions Copyright 2008 Faramarz Jalalian" I just followed the what was done for cmd/expect/Makefile.sfw. Also the comment header instructs that "If applicable, add the following below this CDDL HEADER, with the # fields enclosed by brackets "[]" replaced with your own identifying # information: Portions Copyright [yyyy] [name of copyright owner]" And I wasn't sure if it's applicable or not, so I added the line to be safe. I think it doesn't mean anything other than I have created this file. > > Use predefined '--prefix=' value from Makefile.master. You > could also use the following instead .. > ./configure $(CONFIGURE_OPTIONS) > also for Makefile.master. Done. I hope that the CONFIGURE_OPTIONS remains general enough in future to be applicable to all configure scripts using it. > > You might want to apply these comments from > 'Roland Mainz' ... > > Use "env - ..." instead of "env ...". > > > Use either $(SHELL) or /usr/bin/bash for "configure" > > calls (so you know which one is used and "configure" > > doesn't pick one itself). All good suggestions. Done. > > 2. usr/src/cmd/cmake/METADATA > Add lines for .. > URL: <link to where source code came from> > SUPPORT: ?? > BUGTRAQ: solaris/utility/??? > Done. > 3. usr/src/cmd/cmake/install-sfw > You might want to apply these comments from > 'Roland Mainz' ... > > Use /usr/bin/ksh93 or /usr/bin/bash for install-sfw* and > > add a $ set -o errexit # at the beginning > > and replace ". ${SRC}/tools/install.subr" with > > "source ${SRC}/tools/install.subr" (the idea is to > > catch failures in the script and abort it at that point, > > right now the script will just continue) Done. > > 4. usr/src/Makefile > Why is this in your webrev? > Probably added accidentally. Removed. > 5. usr/src/pkgdefs/SUNWcmake/depend > This looks like the default 'depend file! If you have > no other dependencies then you should remove this file > and add "DATAFILES= depend" to your SUNWcmake/Makefile. > > Have you checked you have no other pkg dependencies? If > you have keep this file and add them here. Also .. > > move Copyright lines to after the "CDDL HEADER END" > header. > I prefer to keep this in case that some kind of dependency is discovered later. I moved the copyright lines. SUNWexpect and SUNWdiffstat also have the same problem. > 6. usr/src/pkgdefs/SUNWcmake/pkginfo > Delete this file as its generated from the pkginfo.tmpl > file. Done. > > 7. usr/src/pkgdefs/SUNWcmake/pkginfo.tmpl > Copyright year is wrong. Done. > > Expand the 'DESC=' line, maybe ... > DESC="CMake - A cross-platform open-source make system (2.6.1)" > Done. > 8. usr/src/pkgdefs/SUNWcmake/prototype_com > & usr/src/pkgdefs/SUNWcmake/prototype_i386 > & usr/src/pkgdefs/SUNWcmake/prototype_sparc > Copyright year is wrong. Fixed. > > === End of Comments ====== >
