James Carlson <james.d.carlson at sun.com> writes: I'm responding to the bits I can as we go, these aren't so much responses as "Here's why I recall things being this way".
> Richard Lowe writes: >> A webrev is available for prelim code review: >> >> http://cr.opensolaris.org/~richlowe/scm-review_90 > > I'll be issuing my comments in batches, in part because of the size of > the review. ;-} > > One high level issue to deal with: the CR should be updated with a > summary of the major components and tasks, so that reviewers have an > idea of what the scope of changes should be. (Otherwise, some of the > changes that I know to be right actually look quite suspicious.) > > usr/src/Makefile.master > > 180: this should be /usr/bin/python > > 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). > usr/src/tools/findunref/exception_list > > General comment: at the moment of integration, we will need to do a > very careful check to make sure that the three new exception_list.* > files contain exactly what was in the old file. The usual merge > effort will _not_ suffice. > > As it stands, it looks like we're currently missing this entry: > > usr/src/uts/sparc/nv_sata/Makefile > > ... which seems to have arrived in this changeset: > > changeset: 4876:ecd69ba0713a > user: mlf > date: Thu Aug 16 14:46:34 2007 -0700 > summary: PSARC/2006/501 Nvidia ck804/mcp55 SATA HBA driver > > (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. 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? > 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. > 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. > 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. > 57: this output represents abject failure; it should go to stderr. > > 67-69: I'm not sure this works. Is it even needed now? > > 87: nit: s/try and/try to/ > > 91: really exit? Why not just return True and continue with the > rest of the checks? > > 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. > I would have expected 'if in_cddl:' as a test to perform outside of > the for loop. > > usr/src/tools/onbld/Checks/DbLookups.py > > It'd be nice if this knew about defect.opensolaris.org. Lack of > direct support of open bug tracking in the new tools is going to be > a stumbling block for external committers. defect.opensolaris.org is not even currently a beta, fervent desire aside. I happen to have code that would support its use, but I have no plan to integrate it currently. The block on external committers to most places, right now, is that bugs in d.o.o don't count for (ON, SFW, etc, etc). Only the various project gates making use of it. > 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. > 368: nit: s/try and/try to/ > > usr/src/tools/onbld/Checks/HdrChk.py > > 35: isn't this an issue for the separate comment checker? > > 46,146: this isn't needed once we get past Teamware. > > 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? > usr/src/tools/onbld/Checks/Makefile > > 56: nit: it looks to me like 'all .WAIT' doesn't do anything here. > Line 48 will establish the .pyc files as things that need to be > installed, and line 61 declares the local .py file to be a > dependency of the corresponding .pyc file. > > 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 > 91: this should probably be "and cr not in badRtis". Once a CR has > been declared "bad," I don't think there's a need to check it again. > > 108: shouldn't an error throw an exception? > > 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. > 56: this will trigger two traversals of the subdirectories -- one > for the "all" target, and another for the "install" target. A > cleaner way might be to omit the "all .WAIT". > > usr/src/tools/onbld/Scm/Backup.py > > Random GPL? See above, and wait on response from Bonnie. > (Eyes glazed over at this point; only very superficial review done.) Gee, thanks. > usr/src/tools/onbld/Scm/Version.py > > 47: I'll echo the comments about "englishify." Suggest "textlist" > or some such. Agree. -- Rich
