On Aug 5, 2009, at 12:58 AM, Vladimir Marek wrote: > Hi, > >> The correct workspace was identified and a new webrev has been >> generated for your review. For all of those who are able and >> interested, please examine the following webrev: >> >> http://cr.opensolaris.org/~patb/pmtools_webrev/ > > * needs resync (as stated by Paul already) > > * Makefile.sfw:46 > 46 INSTALL_ROOT=$(ROOTBIN) \ > 47 DESTDIR=$(ROOTBIN) \ > > That does not sound right at first look. DESTDIR is used as sort of > prefix. So if ROOTBIN is usr/bin, and program would install binary > into > usr/bin/mybinary, result would be in /usr/bin/usr/bin/mybinary. But I > guess you tested this, so you know it works correctly. Just > suspicious. > I understand your point. I was following another example and it works, so disinclined to change it right now. > > * Makefile.sfw:48 > 48 MAKE=$(GMAKE) \ > > This should not be necessary, as make/gmake sets this variable > automatically, so that it points to itself. OK. > > > * Makefile.sfw:66 > 66 -rm -rf $(VER) > > Should be $(RM) -r $(VER) ( see Makefile.master ) I thought about that too, but again, it looked like this in other instances. I'm not opposed to making this change if it's appropriate. > > > * pmtools-01.patch > * New man pages > - Any chance of pushing the changes to the original authors? The patch came from one of the original developers of these utilities and I ran the man pages past him too and he thought they looked good. I don't intend to put these man pages anywhere besides SUNWpmtools. ;)
Thanks for your comments! Pat > > Just nits really > > Thank you > > -- > Vlad
