Roland Mainz wrote:
> 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".
>
You'll need to remove them, and submit the review, before you submit the
RTI. Normally it takes more than 10 minutes for your advocate to
approve them.
The main thing is that at some point you need to "freeze" your sources
and stop synchronizing with upstream until after you commit. That's
"hard", but you need to do it anyway, and it should not matter whether
that freeze lasts for 10 minutes or 10 days.
>
>> 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 confess I didn't follow this ARC case so closely. It got to be *too
many* things to keep track of. Its one reason why I hate having cases
that try to bundle all possible changes into a single "update subsystem
X" case. If the updates are minor, its not a problem. But your case
did much more than that. (But no matter, its already done.)
>
>> (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".
>
OK. Thanks.
>
>> 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).
>
There are test machines available within Sun. You're still inside SWAN,
right? Ask about LRT2 (hint: goto lrt2.sfbay to reserve a machine for
remote access.)
There are some test suites available for running standardized boot time
testing as well. I forgot the URL, but I bet you can find it on the
onnv.sfbay page. (And probably on the OpenSolaris ON community pages as
well.)
>
>> 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.
>
Hmm... I just worry that if anyone might be surprised by the new
format. If its easy to retain the old format, my preference is to do
so, just to avoid the *chance* of introducing a problem, unless there is
a compelling reason for the change. (I'm not sure the ksh93 legacy
behavior counts or not.)
>
>> 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.
>
OK. Thanks.
> [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.
>
Great. But what about ordinary "man"? Having builtin man pages might
be nice (although there are problems that it poses for L10N as well, I
think, so I'm not sure its an unqualified benefit), but we need to have
the same docs in the "normal" man location. (I.e. users that have
decades of typing "man <command>" experience...
- Garrett
