Richard Lowe writes: > James Carlson <james.d.carlson at sun.com> writes: > > usr/src/tools/SUNWonbld/Makefile > > > > 31: unexpected change ... how did we grow GPLv2 on top of the > > existing license? (See findunref.py comments below.) > > cdm, which is intimately familiar with the Hg internals. We talked > this through with Bonnie at the time cdm integrated, and again when I > noticed findunref and hg-active were possibly similar (the mail from > that occurance is on the list in the last couple of months).
Ah, I see. Since hg is GPLv2, cdm needs to be (or should be). That's the connection I was missing; thanks. > > usr/src/tools/findunref/exception_list > > (It'd be a bit simpler if one of these files -- perhaps > > exception_list.open -- was just a rename of the original file. Then > > the merge *be expected to would* catch problems.) > > Right, I'm doing it by hand now, and it's an absolute pain in the > ass. I'm not certain renamifying it after the fact would be good for > us though, so I haven't. It'd force us to deal with the conflicts at merge time. Having it as a separate instance of the file hides those merges. > I agree that they need careful verification, and I apologise for the > mismerges that have no doubt crept in. Would you like to file a bug > regarding that one now, or to do a search and then file one bug with > all problems? I did the best search I could, and that's the only one I found. I've filed bug 512 for it. > > usr/src/tools/findunref/exception_list.closed > > > > Does this file work? It doesn't appear to have complete workspace- > > relative paths in it, the way the other exception_list.* files do. > > Has it been tested? > > I don't know, I don't have usr/closed. > > Note, however, that in the Hg case, we walk the files based on > repository, the closed repository is *rooted* in usr/closed/ > > So the missing usr/closed/ on those lines is correct. I obviously > have no idea how completely this has been verified with TeamWare in > use and usr/closed present. See my own comments on findunref, also. OK. I'll make a note to test this and file bugs if I see any problems. > > 48: this comment doesn't seem to match the code the follows. > > > > usr/src/tools/findunref/findunref.py > > > > How'd this end up being under GPLv2? I suppose I don't care much, > > but it seems like an odd change to me, given that the original code > > was under CDDL, and that the claimed author listed in the code (Sun) > > didn't request a license change. > > See > http://mail.opensolaris.org/pipermail/scm-migration-dev/2008-April/002067.html > > I've asked Bonnie to also respond to you, to make absolutely certain > we got this right. OK. I didn't see the Mercurial bits there, but if that's the issue, then I get it now. > > 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. :-< > > 107: 'exclude' seems to do nothing here; is that intentional? (I'm > > guessing "yes -- because the hg walk doesn't report directories as > > part of the repository.") > > See my own comments, findunref is unintelligible, currently. ;-} > > 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. > > 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? > > 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. > > usr/src/tools/onbld/Checks/Rti.py > > > > 91: I think this could be more efficient. Querying an RTI should > > return a list of CRs (all of the CRs in the RTI), and we should be > > able to check against that list. Most (all?) putbacks have exactly > > one RTI. In fact, querying the database and finding multiple > > different RTIs for the CRs the user lists is probably an error case > > to flag. > > I talked this through with Mark when he was making changes in there, > your request would seem to be bug #496 OK; thanks. > > 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. > > (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. ;-} Richard Lowe writes: > While for a change to slim_install or pkg-gate it would be valid, it's > absolutely wrong for others. The other alternative I can think of > (baking that knowledge into the code) is untenable, I think. As we discussed, my preference is to have the tools support the most "reasonable" environment, not necessarily the most restrictive one. Since (a) we're trying to switch into an open bug tracking site over time and (b) these sorts of bugster-uber-alles assumptions are built into the system in _many_ places, I think this is a slow transition of relaxing the rules. Don't worry. Gatekeepers reviewing RTIs will still be there to say, "sorry, this gate doesn't allow the use of bugs documented in d.o.o; you would have known that had you read the gate README." -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
