hi meem thanks for the comments! all are accepted bar two i have questions about. details foillow..
> General: > > + I'm confused on the convention for SVCMETHOD scripts -- it seems > like sometimes we check-in the .sh file and copy the SVCMETHOD > from the .sh file during the build (such as with inetd), and > other times we just directly check-in SVCMETHOD and install it. > Is there a rationale here? Seems like we should pick a convention > and stick to it. > not sure about the rationale, or which is preferred, i'll ask the smf folks. > * In a number of Makefiles, you've added an include for > cmd/Makefile.targ. That includes a `clobber' target which often > overlaps with the one already defined in the Makefile. This > should be rationalized. > now you mentioned it i noticed a few multiple target warnings and have fixed these. > src/cmd/Makefile.cmd: > > + 238: What does this do? We set ROOTSVCBIN here, then reset it > on line 242. > nothing, as far as i can see. i've removed line 238. > src/cmd/auditd/Makefile: > > + There appears to be quite a bit of brokenness in here that would > be nice to fix up. Specifically, SUBDIRS is never set so it > should be stripped from the targets, lines 75-78 should be > removed, `local_clobber' should be torched, a proper `clean' > target should be introduced, and `clobber' should come from > cmd/Makefile.targ. (Oh, and TEXT_DOMAIN is already set from > Makefile.cmd.) > all accepted. > src/cmd/cmd-inet/usr.lib/Makefile.inetsvc > > + We could finish simplifying things by removing lines 30-31, > 43-46, and 54, and changing lint_SRCS to lint_PROG on line 56. > all accepted. > src/cmd/cmd-inet/usr.lib/in.ripngd/Makefile: > > + 76: Might as well fix this to be lint_SRCS. > accepted. > src/cmd/cmd-inet/usr.sbin/Makefile: > > * 222: Umm, INTSUBDIRS? > oops, fixed, thanks! > src/cmd/cmd-inet/usr.sbin/in.ftpd/Makefile: > > * 12-13: While this probably works, it would be more consistent > to hoist PROG and SCRIPTS as well. > i'm not really sure what you mean here to be honest. can you elaborate a bit? thanks! > src/cmd/cmd-inet/usr.sbin/in.rdisc/Makefile: > > * 27: Initial include of Makefile.cmd seems unnecessary. > true. > * 32-34, 44: None of this stuff should be needed since this is > a single-source-file program. > ok. > * 42: Are all of these CPPFLAGS really needed? Why? BSD_COMP and DSYSV are needed, however no files in the specified include directory seem to be used so that can go. > > * 48: Makefile.targ should come at the end to avoid overriding > the default target of `all'. > accepted. > src/cmd/cmd-inet/usr.sbin/in.routed/Makefile: > > * 78: Why include Makefile.targ here rather than at the end? > i think i forgot to revisit these files to fix the method stuff to be honest. fixed now. > src/cmd/cmd-inet/usr.sbin/routeadm/Makefile: > > * 30: Remove blank line. > > * 34-37: None of this stuff should be needed since this is a > single-source-file program. > both accepted. > * 46: Are all of these CPPFLAGS really needed? Why? > on checking, only one needed is the include of the common/svc subdirectory. i've removed the rest. > src/cmd/consadm/Makefile: > > * 58-60: Can't this rule be removed as well? > yep, missed this one for some reason... > src/cmd/fs.d/nfs/svc/Makefile: > > + Make `all lint' the first target. > sure. > src/cmd/ipf/svc/Makefile: > > + Would be nice to change lines 17-23 `all clean lint:', since > there are no source files. sure, and i moved include of Makefile.targ to the end also. > > src/cmd/lp/cmd/lpsched/Makefile: > > + 29: Might as well fix this pathname. > > * 38: Remove duplicate MANIFEST setting. > > + 154: Could just be lint_SRCS. > all accepted. > src/cmd/print/lp/Makefile: > > * Why does this file show up as "new"? > it was removed from ON, i've updated my workspace now so it's gone in the updated webrev. > src/cmd/lvm/md_monitord/Makefile: > > * 92: Is this ROOTMETHOD rule still needed? > nope, removed it. > src/cmd/lvm/*/Makefile: > > + I notice all of these LVM Makefiles have a strange disease: > they all have `-$(RM) $(ROOTPROG)' in their `install' rule. > This makes no sense to me. > yeah, ROOTPROG is not even defined as far as i can discern. i've removed these. > src/cmd/lvm/rpc.metamedd/Makefile: > > + 43: `check' doesn't descend into SUBDIRS, so this can be > removed. sure. > > src/cmd/oplhpd/Makefile: > > + 52: Take out the extra blank line too, and the two extra ones > on lines 55-56 while you're at it. > > + 70: This `clean' rule is wrong; it should be `$(RM) $(OBJS)'. > all accepted. > src/cmd/picl/picld/Makefile: > > + Lines 30-34, 40-41, 65-66, and 71 all appear needless since > this is a single-source-file program (and line 74 should be > lint_PROG). > all accepted. > src/cmd/print/gateway/Makefile: > > * 114: This should be ROOTSVCMETHOD -- but the whole thing > looks pretty out-of-date. How about we just torch 103-114? > sure. > src/cmd/rpcbind/Makefile: > > + 64, 66-68: This DIRS handling is cruft. > yep, i've removed it. > src/cmd/rpcsvc/Makefile.rpc: > > * 33-34: This sentence is incorrect and probably unnecessary. > ok, i'll remove it. > * 40-48: Why not just "ROOTMANIFESTDIR= $(ROOTSVCNETWORK)/rpc"? > I don't grok what these additional rules are doing. > you're right - they actually trigger a "too many targets" warning in subdirectories since we end up with 2 $(ROOTSVCNETWORK)/rpc targets. > src/cmd/rpcsvc/rpc.bootparamd/Makefile: > > * Why was this file changed? > shouldn't have been, mistake on my part. > src/cmd/sa/Makefile: > > * 113-119: Why are these conditional assignments still here? > shouldn't be, i've removed them... > src/cmd/xntpd/xntpd/Makefile: > > + 12: The `all' rule should really be moved down to around > line 24 to be more traditional. > sure. > src/cmd/ypcmd/Makefile: > > * 43, 258: Why hasn't ROOTMETHOD been removed and line 258 > changed to ROOTSVCMETHOD? > oversight on my part, fixed now. > src/cmd/zoneadm/Makefile: > > * 34, 52: Likewise. fixed also. updated webrev: http://cr.grommit.com/~amaguire/onnv-quagga-updated-2/ if those changes look okay, we'll close out the review. thanks again! alan This message posted from opensolaris.org _______________________________________________ networking-discuss mailing list [email protected]
