James Carlson <james.d.carlson at sun.com> writes: >> > 41,42: I don't think we should have exceptions here for SCCS >> > directories or for Teamware ".del-*" files, as they'd represent >> > errors if present. Perhaps these are things that `nightly' could >> > add with a findunref command line option just when doing >> > Teamware-based builds. >> > >> > 44: this one is odd. I would think that if we need to exclude the >> > *.flg files, we'd do that in the explicit exclusion lists, rather >> > than as a hard-coded feature of findunref. They're just Teamware >> > artifacts, and they should eventually go away. >> > >> > (Perhaps you could address this with a post-integration bug to have >> > this code removed.) >> >> >From memory, those exclusions were pulled directly from findunref. > > Still ... I don't think putting SCM assumptions into the tool makes > sense unless the tool knows something about the SCM in use. (I > suppose findunref could do that ...) > > As for the *.flg files, I might have to retract that comment for now. > The xref (cscope) tools use those *.flg files and make assumptions > about Teamware. Oh well. :-<
The flg files are used in general, hence the changes to make flg.flp understand mercurial. That puzzled me somewhat too, but I don't see a better alternative to pull sundry other pieces into tags files or the like. >> > usr/src/tools/onbld/Checks/Cddl.py >> > >> > 87: I'm no Python expert, but what does this 'else' pair with? It >> > looks like it pairs with the 'for' at line 71, but that doesn't make >> > sense to me. What does 'for ... else' mean? >> >> That is indeed not useful. However for ... else is a valid python >> construct, the else will run if the loop terminates via exhaustion, >> rather than being break and such. > > Weird. OK; thanks. > >> > I would have expected 'if in_cddl:' as a test to perform outside of >> > the for loop. > > That's the part that still puzzles me. We walk the entire file, line-by-line, looking for blocks. I'm not sure how we could move that check outside the loop, given whether or not we're expecting to be within a block varies. (am I not looking at the right code?) >> > usr/src/tools/onbld/Checks/DbLookups.py >> > 234: this is missing onSWAN support. It should be looking at either >> > at the NFS-mounted file /shared/sac/<ARCNAME>/<YEAR>/<CASE>/IAM.*, >> > or at 'http://sac.eng/arc/' followed by the usual <ARCNAME> and >> > such. >> >> That's intentional. We only support separate SWAN and not SWAN paths >> where results may differ. In this case, results should never differ, >> so the SWAN path is useless. > > There are both open and closed ARC cases, aren't there? This is... thorny, I thought the current behaviour was generally known, but when it's been mentioned to others they've been surprised (and in some cases unhappy). Some ARC cases are exposed and some are not, however, the case number and title of *every* case is exposed. The exposure flag toggles whether the actual materials are visible. So in the case of ARC (as opposed to pretty much all the others), we actually have enough information on opensolaris.org alone. >> > usr/src/tools/onbld/Checks/Keywords.py >> > >> > 44: this will accidentally match (and identify as an "SCCS keyword") >> > perfectly valid things such as: >> > >> > printf("foo %X%s", hexval, "\n"); >> > >> > I'm not sure what to do about that problem, but I think that as we >> > get further and further away from SCCS, we will want to rethink >> > this. >> >> That's the classical annoyance with SCCS keywords, it's basically >> impossible to separate them from (some) valid language constructs. >> >From memory (unless fixed), SCCS itself does nasty things in those >> cases, doesn't it? > > Sure. I'm worried about code going forward from now. Once the gate > is all Mercurial, I shouldn't have to worry about putting something > like the above back. Today, I have to do this sort of subterfuge: > > printf("foo %X" "%s", hexval, "\n"); > > Being able to get rid of that nonsense seems like a benefit to me, and > it'd be nice if the tools didn't have an SCCS hangover. The hangover exists so existing uses of keywords can be called out and removed. It should be short lived. Is there an alternative that means it isn't needed at all? We could look for #pragma ident, but that won't call out uses in other places, not all of which we intend to remove during migration (do we?). If there's a way to avoid this, I'd like to. >> > usr/src/tools/onbld/Makefile >> > >> > 47: doesn't seem to be used. (?) >> >> >From memory, the basic definition of clobber that we would inherit >> depends upon 'clean', so this would do what we want. That needs to be >> verified however. > > ... in which case, it would end up setting TARGET to 'clean' as it > built that dependency. Nothing is done with TARGET=clobber as far as > I can tell. Ah, now I get it. >> > (Eyes glazed over at this point; only very superficial review done.) >> >> Gee, thanks. > > Nothing personal, of course. Just too much Python to digest in a code > review, particularly for someone who doesn't know the language well. ;-} It wasn't meant to be taken so seriously :) Thanks, -- Rich
