Thank you. I have addressed these comments and the one other that I got privately.
The incremental webrev of the affected files is here: http://anthrax.central/net/mrliberal/export/ws/mjnelson/onnv-transition-dev/webrev.incremental.codereview/ (also available at http://cr.opensolaris.org/~mjnelson/webrev.incremental.codereview) ...and my responses are inline below. --Mark >> http://anthrax.central/net/mrliberal/export/ws/mjnelson/onnv-transition-dev/webrev.transition/ > [...] >> I need review of the "interesting stuff" and "6729074" links, and I would >> be thrilled if somebody besides me took the time to wade through the rest. > > Despite the clear warnings otherwise, I went through all of it. ;-} And, as I promised, I'm thrilled. :) Even though I said it above, I'll repeat it here: thank you, Jim. This is great. > In general, the CRs and other bug entries need evaluations and the > other usual attributes. They're mostly stuck in "Accepted" state or > something like that. Yes, sorry about that, I'm updating 'em all this morning. I'm also filing a single bugster bug to use as a bucket for the stuff we tracked via bugs.grommit.com. I'll send out links to each of the five bugs when I get the bookkeeping work done. > In the Python code, I did what I could, but I'm no Python expert. The > one thing that jumped out at me was the funky spelling of 'honour'. > (Just a nit, but the tradition in at least ON [and probably other > consolidations as well] is to use American English spellings rather > than "International" English. If this were user-visible, I'd call it > a bug rather than just a nit.) Because they were comments, and not generally user visible, I elected to honour the author's preferred spelling. :) > For 444 (.hgignore), the fix (apparently just ignoring everything) > doesn't seem to match the description in the bug report itself. I added a comment to the bug. > For the .java files, do the "@version" things matter in any way for > javadoc? If they weren't important, then just stripping them seems > good. Otherwise, we might need some other fix. (I'm asking because > [a] I'm no java expert and [b] CR 4758439 is still in 'Accepted' state > and doesn't have an Evaluation.) They "matter" to javadoc, in that they cause false positives on a wsdiff. I do not believe that they matter for anything useful. > For pkgdefs/SUNW{mibii,sacom,sasnm}/pkginfo.tmpl, the PSTAMP entry was > entirely removed. This is usually commented out like so: > > #PSTAMP="<developer defined>" > > I'm guessing that it doesn't actually matter, but I'm not sure what's > done with this upstream during WOS construction. (I looked at > SUNWmibii in the delivered product, and pkginfo there has the swilly > string, so this will make an observable change. Probably for the > better, but may need to be at least disclosed in the CR.) I will need (as I previously promised Rich) to upgrade test these packages. In reviewing this change initially, I checked for the absence, comment, or definition of PSTAMP in ON packages, and I found a sufficient number of each case that I didn't feel strongly, especially since "absent" and "commented" are functionally equivalent. > For tools/codereview/codereview.1, and as long as you're changing this > anyway, I suggest fixing line 22 to look more like the other headers. > In other words: > > .TH codereview 1 "10 June 2005" > > ... lower-case name, section 1 (not fictitious "1-LOCAL"), and spelled > out date rather than funky SCCS reversed date. Done. > A similar comment applies to the change in tools/scripts/acr.1:22. Done. > tools/cw/cw.c:1864: if this "-_versions" thing is needed at all, then > I recommend making that version string a #define, and defining it near > the top of the file along with a block comment. If it's useful, then > it'll need to be updated each time someone modifies the file, and > burying it in the code makes it unlikely that'll happen. I changed this to a #define, and also filed: 6733832 Need to revisit version output of compiler wrapper script > tools/env/opensolaris.sh:82: the "//hg" here doesn't look right to me. > All of my working paths to hg.opensolaris.org use a relative reference > to "hg/onnv/onnv-gate" and not an absolute path. Is this right? Because hg.os.o does a chroot, absolute and relative paths are functionally equivalent. Rich and I were surprised to find each other using different forms of the same path, but I tested both, and each works. I have a slight preference for the absolute, because I wonder if the chroot will someday go away, and if so, I think that means the paths won't need to change, as long as we keep the repos in the obvious location. > tools/scripts/wx.sh:28-29: these can probably go now as well. Done. > I don't see how the change in tools/onbld/Scm/Backup.py is related to > bug ID 303. It's this single line (from the new file): 363 self.files = ('hgrc', 'localtags', 'patches', 'cdm') that adds the new .NOT files in the .hg/cdm/ directory to the list of files to be included in a backup. > I like that some of the things you nuked were old, broken, expanded > SCCS keywords in the SLP code. ;-} Thank you. I like a lot of what we're doing, too. :) --Mark
