Bill Sommerfeld <sommerfeld at sun.com> writes: > Jim Carlson asked me to review what I could of the big webrev at: > > http://cr.opensolaris.org/~kupfer/onnv-scm/
I'm probably going to have more comments on these, but I'm going to respond to those I can respond to quickly now. > This is my initial batch of comments, focussing on changes to existing > scripts in reverse alphabetical order.. > > usr/src/tools/scripts/xref.sh: > > WES-1 detect_scm(): shouldn't this be in some sort of common code? > rather than cut & pasted and separately maintained? Yes, it should (for some reason I answered this below...) > WES-2 detect_scm() logic: for a hypothetical broken workspace where > both .hg and Codemgr_wsdata exist, should the presence of "workspace" > in the path determine whether to prefer hg over teamware? Should investigate, and document. > WES-3 Should we set CODEMGR_WS in the non-teamware case? We've been back and forth a few times on the use of CODEMGR_WS, my view (which I think was shared by stevel) is that CODEMGR_WS, while originally used by and for teamware, has effectively become generic at this point, and that picking different variables to hold the workspace root merely because the SCMs differ would create yet another thing for users and the tools to keep track of with little benefit. > usr/src/tools/scripts/wx.sh: > > WES-4 I'm a little confused about why lines other than 4417-4424 (the > code which disables wx if hg finds a root) are part of this > wad. I'm taking that to mean "Why did wx change, other than to complain about hg?". The original plan (sch, stevel's, and to a lesser extent mine) was that to the greatest degree possible everything should use a common set of code for the checks, which in this respect was the new code. > WES-5 Is it possible that this will trigger a false positive if you > run "hg init" in a parent directory of a teamware workspace? > Where do we document these limitations? Should investigate, and document. (I can run wx, but not in any 'real use' situation, I'd appreciate someone with more useful access to these things would check that out.) > usr/src/tools/scripts/ws.sh: > > WES-6 As with WES-1: we seem to have more open-coded SCM system > detection which is *different* from that found in xref.sh Yes, that should be made common, either sourced into the scripts or built into the scripts in the same way as nightly/bldenv have stdenv.sh baked into them. > WES-7 line 371: I think you mean "SunOS 5.x-specific" since 5.0 is > long gone. Accept, I got stevel to remove the 4.x bits, but didn't think to have it say 5.x rather than 5.0. Oops. > WES-8 Lines 376-408: I suspect a "uname -s" check is also in order. > > usr/src/tools/scripts/webrev.sh > > WES-9 Lines 1150 "comments_from_wx" seems wrong for mercurial if > wx commits suicide if mercurial is in use. The mercurial support generates a file list in wx format, and passes it down. We've had a request from the xVM guys to make that no longer be the case, but currently comments_from_wx is referring far more to the file format in use than the tool. We should make that clearer though. > WES-10 Lines 1469-1497: yet another "detect_scm"!! Gotta catch 'em all! > WES-11 Lines 2435-2448: doesn't hg have a different way of recording the > committer's name in .hgrc ? Shouldn't we use that if available? It does, and that specifies the author name in the changeset. Do you mean that we should attempt to pull the information that Hg would make use of out of their settings, or that we should attempt to reflect the info in their actual changeset (as they may differ). (if there's more than one changeset, there maybe more than one author, too. I'm not sure how that would fit in) > usr/src/tools/scripts/sdrop.sh > > WES-12 Lines 55-57: yet another detect_scm fragment which doesn't use "hg > root" > but instead checks for .hg ?? > (and, uh, $CODEMGR_WS/.hg ??!?!?) As above, the current view is that $CODEMGR_WS and $CODEMGR_PARENT have become genericised as "workspace" and "parent", neither, necessarily under CodeMgr/TeamWare control. If there's strong enough disagreement from enough people would could attempt to change that, I guess... > usr/src/tools/scripts/nightly.sh > > WES-13 I thought bringovercheck was going to be nuked as part of this wad. That was my original plan, yes. But we will be integrating to onnv while TeamWare is still in use, both for ON and SFW, bringoverchk has to stay around until all the gates using it are no longer using teamware, at least. > usr/src/tools/Makefile.tools: > > WES-14 Lines 66-73: might INS.pyfile belong in Makefile.master (we're going > to > have python elsewhere in ON at some point, right?) I wasn't sure what to do about that, so I took the more localized approach. I'm happy to change it. Thanks for looking at all these. -- Rich
