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.) 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.) 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? 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. 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.) 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.") 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? 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. 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. 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. 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. 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. (?) 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? (Eyes glazed over at this point; only very superficial review done.) usr/src/tools/onbld/Scm/Version.py 47: I'll echo the comments about "englishify." Suggest "textlist" or some such. -- 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
