Garrett D'Amore wrote: > Roland Mainz wrote: > > <NOTE>Please note the _timeout_ is set to 2008-08-25 since we don't have > > much time left anymore, after that point the code will move to a > > Mercurial tree for final review and integration.</NOTE> > > > > Here comes round "two" of the preliminary code review for the first > > update of the ksh93 integration project: I created a webrevs based on > > today's (2008-08-22) ksh93-integration > > prototype012 Subversion tree which is based on the OS/Net Mercurial HEAD > > version. > > > > The webrev can be found at > > http://cr.opensolaris.org/~gisburn/ksh93_integration/ksh93_update1/webrev_20080821_001/ [snip] > > Unfortunately, I can't really help out with the code review, but I will > offer up a few comments/questions/suggestions, based solely on the > message below. > > 1) You should remove ident lines now -- you won't be allowed to put them > back and fix them later. (Not unless you get some kind of special > waiver from ON CRT, and I really don't think one should be granted.)
Umpf... for the Makefiles+code we "own" (e.g. usr/src/lib/lib(shell|cmd|dll|sum|pp|ast)/, usr/src/cmd/ksh/ etc.) I'll remove the "ident" lines later today... for the other Makefiles I would strongly perfer to "defer" the removal of these lines to a very very late point (10mins before putback ? :-)) since this is a major source of "bitrot". > 2) As far as your builtins go, I have a couple of concerns, which *may* > come up at ARC. Erm... all code in the "prototype012" tree matches the ksh93/AST+ksh93/AST update1 ARC cases. > So by properly answering these ahead of time (or at > least knowing the answers), you'll save trouble later at ARC: > > a) Are you 100% certain that the builtins are 100% drop-in > compatible with the old versions? Yes, we do (there is one issue with /usr/bin/sleep's SIGALRM signal handling pending, a patch for usr/src/lib/libshell/common/bltins/sleep.c is being worked on). The code patches the standards and the ARC cases as specified. > (I'm not talking about adding new > options -- I'm saying no change where the old commands did *not* > generate an error of some sort.) Note that I suspect for printf at > least, you'll have some formats which now will be handled differently. > For example, printf "%z" prints "z", instead of returning an error. > While it would be insane for anyone to rely on that behavior (and so it > shouldn't be a problem at ARC), any new format specifiers or > interpretation would need to be documented in the ARC materials. This has been done in the ARC case, including the comment that relying on undefined %-formats puts you in "hot water". > b) The normal perf. concerns. The effect of your change may be > positive, but some kind of analysis would be useful. (And I'm not > talking about your atypical scientific computing applications. I'm > talking about impact to things that folks are already doing. A good > perf. test might be building ON -- with *stock* Makefiles. Just because > you can tune it for ksh93 doesn't mean everyone will do so, so what I > want to know is how "untuned" applications will behave. Boot time > analysis may be helpful as well. I can't provide boot time analysis since I am mainly using virtualised environments (like VMware) which are not 100% reliable for peformance testing but OS/Net builds a bit faster (not much but we definately not regress the build time). > c) kill -l change is slight, but seems also to be "arbitrary". Is > there a reason that the old SPACE behavior isn't retained? That was discussed in the ARC case. The POSIX standard allows both (Don Cragun checked and confirmed this) and ksh93 and the AST toolkit output the one-per-line syntax since the beginning. And it's easier to parse. > 3) I've not looked at sum, but are you making use of (or later plan to > make use of) the encryption framework in Solaris? /usr/bin/sum will use libsum which uses libmd on Solaris for those hashes supported by libmd. [snip] > 4) Will we finally get open man pages for these commands as well? "man > type" is not terribly useful on OpenSolaris 2008.05. Try $ type --man # ... all ksh93/AST commands support the options "--man" and "--nroff" to obtain/generate manual pages. ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;)
