Jim, My responses are inline. I split your code review into 3 sections and will be sending the responses out as I finish them. I will also generate a new webrev and send out the link to it.
thanks, Moriah James Carlson wrote: > > This isn't intended as the right long-term answer, is it? We've tried > to avoid static libraries, even for internal use. It seems to me that > this should be factored into a normal shared library. (Yes, I realize > that things like the weird quit() function need some minor redesign to > work properly.) > >> http://cr.opensolaris.org/~mwaterl/6739234.changes_only/webrev/ Filed 6839397 libinst should be made a dynamic library Please, see Note 1. > > General comments: > > The "/*LINTLIBRARY*/" token doesn't belong in any .c file. If you > see one there, and you're editing the file anyway, please remove it. > Fixed, removed /*LINTLIBRARY*/ token from o 21 files in libinst o 26 files in libpkg > Make sure you do a "make clobber" build and then check your > workspace with 'hg status'. I think some of the clobber targets > will remove things that 'hg' knows about -- either because of bad > clobber targets or because of things checked into 'hg' that > shouldn't be there. > Done. I completed "make clobber" builds and verified that only the correct files were in hg. I ended up introducing several of these problems only in my webrev workspace that I created for the code review. > Comments: > > usr/src/cmd/Makefile.cmd > > 62,140,144: this already exists; suggest using existing $(ROOTBIN) > and related symbols instead. Done. > 144,145: duplicated line? Removed this. > 328: stray "XXX" Removed this. > 329-330: this isn't right; it expands as: > > $(SCRIPT:%=$(ROOTUSRSADM)/%)/%: % > > ... which means that you're addressing targets that are > subdirectories of the members of the $(SCRIPT) variable, and > no such thing is delivered. Removed this. I left the targets for delivering pkgscripts in the Makefile in that directory > usr/src/cmd/svr4pkg/Makefile > > 37: suggest alphabetic order. Done. > 71-73,91,93-94: this shouldn't be needed; add new directories to > Targetdirs instead. Done. > 87,90: stray; please remove. Done. > 88: clean, clobber, and lint targets do not need .WAIT; only "all" > does. Removed the .WAIT as part of cleanup to address other code review comments but I end up adding a dependency (Please see Note 2): > $(CMDSUBDIRS): $(LIBSUBDIR) The behavior of .WAIT is still present for the clean, clobber, and lint targets but I am not addressing this issue further because my new fix is maintainable, works correctly, and does not significantly contribute to the overall run time for builds in the svr4pkg directory. > 91: this doesn't look right to me; is "install" supposed to build > "_msg"? Doesn't this result in building "_msg" twice? (Could > eliminate special install target and just fold into same for 'all'.) Fixed, please see Note 2. > 97-98: $(SUBDIRS) is never set. Fixed, please see Note 2. > 103-104: stray XXX comment; please remove. Done. > 104: nit: spelling error Done. Removed comment. > 105,108: nit: merge as: > > $(LIBSUBDIR) $(CMDSUBDIRS): FRC > @cd $@; pwd; $(MAKE) $(TARGET) > Fixed this as part of addressing other comments about how I defined and used the SUBDIR targets in svr4pkg/Makefile. My fix is as follows: > SUBDIRS= $(CMDSUBDIRS) $(LIBSUBDIR) . . > $(CMDSUBDIRS): $(LIBSUBDIR) > > $(SUBDIRS): FRC > @cd $@; pwd; $(MAKE) $(TARGET) > 111: why are the dependencies for clean and clobber defined again > here? (Why not define them here and remove them entirely from line > 88?) > Fixed, please see Note 3. > 114-115: unnecessary; creates duplicate targets with duplicate > rules. > Note 1. ------ I filed a bug to make libinst a dynamic library, but my assessment of the likelihood of this actually being fixed is not encouraging. Note 2. ------- done. I gutted this Makefile. I removed all of my debugging comments. I added the new directories to Targetdirs and modified targets to: SUBDIRS= $(CMDSUBDIRS) $(LIBSUBDIR) all clean clobber install _msg lint: $(SUBDIRS) $(CMDSUBDIRS): $(LIBSUBDIR) $(SUBDIRS): FRC @cd $@; pwd; $(MAKE) $(TARGET) You are correct regarding _msg target. I changed this so that I am not even bothering with the additional effort to create a common .po file. I changed the targets in each of the command subdirectories to be $(MSGDOMAINPOFILE) Note 3. ----- Although, there is a "clobber" target defined in usr/src/cmd/Makefile.targ there is NOT a definition for "clean". I removed all the definitions for clobber under svr4pkg and instead added files to CLOBBERFILES +=
