David.Comay at Sun.COM wrote:
> > Here comes round "two": I created a couple of webrevs in various
> > flavours based on today's (2007-02-02) ksh93-integration sources. This
> > isn't the "preliminary review request" (yet), just another attempt to
> > provide an updated webrev.
>
> > * Webrev over all files:
> > http://www.nrubsig.org/people/gisburn/work/solaris/ksh93_integration/ksh93_integration_prototype004_webrev_20070202/allfiles/webrev/
>
> Thanks for providing the webrev. I've enclosed my detailed comments
> below but I also have a couple of questions or general comments:
>
> 1. How exactly are the Mamfiles used? Or is this just files
> included with the ATT distribution for their build system.
Yes, it's coming from the upstream sources... remember we don't want to
fork() the upstream sources and that includes random stripping, file
deletion, renaming of binaries just because someone doesn't like that
name (we had that discussion for "shcomp" a while ago) and all the other
"funny" ideas what could be done to the codebase (like making all
upstream sources "cstyle" clean etc.) ...
> 2. I agree with Jim Carlson about the setting of SHELL in the
> various ksh-related Makefiles. At this time, I think using the
> crufty old /bin/sh is the right thing to use for consistency,
"Consistency" is IMO no good argumment in this case. We use ksh to
simplify things and make the code smaller and better understandable.
IMHO a clean design using ksh is better than a painfull and unreadable
amount of hacked bourne shell code garbage.
> unless some compelling reason can be shown where a ksh (or
> ksh93) construct greatly simplies things.
At least the Makefile.testshell part won't work without ksh and the
include files and l10n generation depends on a lesser extend on ksh
(which means that all Makefiles in
usr/src/lib/lib(shell|cmd|dll|pp|ast)/ depend on it). Both could be
re-implenented using the plain bourne shell at the expense of making the
code really difficult to understand and unreadable (I already agreed to
adjust the directory creation in usr/src/lib/libast/ at the expense of
cleanness in the code and I am not happy about it (it's less
understandable and slower) ... ;-( ).
I would really strongly perfer to keep SHELL=/bin/ksh - it may even
provide the "proof" that we can (later) to a tree-global switch to a
POSIX shell without causing any harm.
> 3. With respect to the *.so library links and the the lint
> library, I think including these in an internal package is fine
> but I do not believe they should be actually included in any
> metacluster at this time. From my understanding of the
> contents of the SUNWastdev packages, that also holds true for
> it as well.
Erm, the primary purpose of the "SUNWastdev" package is to deliver AST
development tools to /usr/ast/bin/. It was not intended to become a
dumping ground for everything.
The putback aims at putting all parts into their permanent locations (I
am looking at the next five or ten years), including SUNWastdev (which
is a developer package).
For the same reason we're delivering libshell/libcmd/libdll/libpp/libast
with the initial putback. In theory it would've been easier to dump all
sources into usr/src/cmd/ksh/ but that would require future movements of
code in the OS/Net tree once we make the matching library APIs public
(and I am not a huge fan of skipping that work and therefore causing
more work to get things moved in the future).
> When the commitment level of these things are
> raised in the future, then it makes sense to include them in a
> package like SUNWcslr, SUNWarc, a metacluster, etc.
>
> Please understand that this isn't a criticism or knock against
> ksh93 - the same is true of internal tools, libraries, etc as
> well.
I don't take this as "criticism" or "knocking against ksh93" (nor do I
think that you or James have anything against ksh93 etc.). I know the
rules... but I was hoping for some kind of "escape route" since many
people are curious and like to play around with the new libraries.
Unfortunately the rules do not seem allow some kind of curiousity... ;-(
> 4. Except for some source files with names that indicated
> "Solaris", it was difficult to know which of the non-Makefiles
> were either not derived from the ATT source or were modified
> from the ATT source. Can you provide a list of which files
> have been modified? Or is it exactly the list of files changed
> by the "diff" file?
That's the purpose of the *.diff files. The *.diff files contain the
_exact_ diff of changes we made to the AST codebase (with the exception
of the all-new test usr/demo/ksh/tests/sun_solaris_getconf.sh and the
demos in usr/demo/ksh/fun/).
> And in those cases, have you added the
> appropriate Copyright to each of those files?
What do you mean with "appropriate Copyright" (note: I am not a
lawyer... (and usually I try to hide under my desk when the license
flamewars start...)) ? AFAIK we do not need (and should not need) to add
any CDDL license to the AST codebase since we have a permission to
contribute any Solaris code to upstream (Mike Kupfer can explain that
AFAIK better) under their own license ("CPL") which AFAIK covers any
tree-local modifications. The new tests and demo code has a CDDL
license.
> usr/src/cmd/ast/msgcc/Makefile
>
> Line 40 - Please make this line fit within 80 columns, using
> string concatenation for example. Something like
>
> CPPFLAGS = \
> .
> .
> .
> '-DUSAGE_LICENSE="\
> [-author?Glenn Fowler <gsf at research.att.com>]\
> [-copyright?Copyright (c) 2000-2007 AT&T Knowledge Ventures]\
> [-license?http://www.opensource.org/licenses/cpl1.0.txt]\
> [--catalog?msgcc]"'
>
> should do the trick.
Quick tests shows that this doesn't work since it inserts spaces... ;-(
I've used the following construct instead:
-- snip --
CPPFLAGS = \
$(DTEXTDOM) $(DTS_ERRNO) \
-I$(ROOT)/usr/include/ast \
-I../../../lib/libpp/common \
-D_PACKAGE_ast \
'-DUSAGE_LICENSE=\
"[-author?Glenn Fowler <gsf at research.att.com>]"\
"[-copyright?Copyright (c) 2000-2007 AT&T Knowledge Ventures]"\
"[-license?http://www.opensource.org/licenses/cpl1.0.txt]"\
"[--catalog?msgcc]"'
-- snip --
Fixed.
> usr/src/cmd/ksh/amd64/Makefile
>
> Line 43 - Although it's isn't documented (although I'm working
> to address that), the preferred ON style here is for the "do"
> to appear on the same line as the "for" as in
>
> for i in $(USRKSH_ALIAS_LIST) ; do \
> [ $$i = $(PROG) ] && continue ; \
> $(RM) "$(ROOTBIN64)/$$i" ; \
> $(LN) "$(ROOTBIN64)/$(PROG)" "$(ROOTBIN64)/$$i" ; \
> done \
Fixed.
> usr/src/cmd/ksh/i386/Makefile
>
> Line 37 - I don't understand the comment regarding /sbin here.
That is a leftover from the day where we had support to install a
32bit-only version of ksh93 at "/sbin/ksh93".
Fixed (comment removed).
> Line 43 - Same comment as above concerning the "for" style.
Fixed.
> usr/src/cmd/ksh/Makefile
>
> Line 70 - Same comment as above concerning the "for" style.
Fixed.
> usr/src/cmd/ksh/Makefile.com
>
> Line 77 - Same comment as above concerning fitting the line
> within 80 columns using string concatenation.
>
> Lines 83-87 - I have my doubts on whether large pages really is
> necessary here
It is not "neccesary"... but it's usefull. Remember ksh93 has no longer
any restriction of the size of arrays (and comes with a new type of
array (which can be used to built large variable trees)) and we even
ship a 64bit version to make sure that we don't create an artificial 4GB
barrier for processing data. Based on benchmarking on an Ultra5/333MHz
and a Blade1000 64k pages seem to be the optimum for small/medium-sized
datasets, 512k pages are useless because Niagara machines don't support
them and 4M pages are an overkill and show a much smaller improvement in
performace (mainly because we get much fewer data mapped with 4M pages,
most smaller allocations end-up in 8k pages in this case (and forcing 4M
pages for the stack would IMHO generate a huge tax for |fork()|'ing of
child processes (64k pages are again the optimum in this case))).
Finally we've done some minor performance work (for libcmd's builtin
commands, more is on the ToDo list to crawl after all temporary
allocations) like putting more data+allocations on the stack to make
good use of the 64k pagesize on stack.
> but assuming it does, why not on the i386/amd64
> side as well?
i386/amd64 has no 64k pages. Using 4M pages had a much smaller effect on
SPARC (and may cause a horrible price for a fork() since the normal heap
usage is far lower than 4MB) and I doubt it will be much different on
x86. I can't verify this because benchmarking this in a VMware VM is
more or less useless.
> usr/src/cmd/ksh/Makefile.testshell
>
> Lines 97-136 - It looks like some of these lines also exceed 80
> columns.
> That said, it would be nice if the actions were
> indented one full tab stop (some are indented by only a couple
> of spaces) and that lines be broken up so that it fits in the
> 80 column width.
I am not sure whether much can be done in this case becase the string
literals are that long and cannot be split-up easily (not without making
the code look like total garbage). I've avoided the usage of TABs since
this would damage the layout of the multiline if/then/fi-constructs and
that looks very bad and is would be much harder to understand what the
code does in this case.
> usr/src/cmd/ksh/Makefile.testshell
>
> Line 43 - Same comment as above concerning the "for" style.
Fixed.
>
> usr/src/cmd/ksh/sparc/Makefile
>
> Line 37 - I don't understand the comment regarding /sbin here.
Fixed.
> Line 43 - Same comment as above concerning the "for" style.
Fixed.
> usr/src/cmd/ksh/sparcv9/Makefile
>
> Line 43 - Same comment as above concerning the "for" style.
Fixed.
> usr/src/cmd/Makefile
>
> Line 24 - Update copyright year.
>
> usr/src/lib/libast/amd64/Makefile
>
> Line 31 - Is there a reason you're only using the minor part
> here rather than the whole $(RELEASE) string? Is there some
> standard here that ksh93 uses with different OS versions?
See may reply to James Carlson's email
(http://mail.opensolaris.org/pipermail/ksh93-integration-discuss/2007-February/002225.html).
Basically the upstream code uses this as a platform identifer and we use
the same algorithm to provide the same value in this case (the code
doesn't build without a value and I don't want to pass a hardcoded
value).
> usr/src/lib/libast/common/llib-last
>
> Lines 36-140 - Please just include the results and don't embed
> the script inside the comment.
Fixed.
> However, if this lint library is not going to be delivered (I
> believe it should not until the interfaces are upgraded), then
> it's not clear why you're delivering this at this time.
I've remove the lint libraries from the package database (and moved them
to the exception list).
> usr/src/lib/libast/i386/Makefile
>
> Line 31 - Same comment as above about using only the minor part
> of the release value.
See above.
> Line 48 - In general, external references to a bug are frowned
> upon in the source. Rather than having the reference, simply
> include a brief summary of the issue at hand (which it looks
> like you already have.)
Fixed.
> usr/src/lib/libast/Makefile.astinclude
>
> Line 41 - s/am Intel/an Intel/
Fixed.
> Lines 54, 57 - s/Solaris/OpenSolaris/
Fixed.
> usr/src/lib/libast/Makefile.com
>
> Lines 682-708 - It's not necessary (and rather distracting)
> having all of the warnings actually listed. Just set CERRWARN
> and be done with it (yes, you can leave the comment about
> eventually this will be fixed upstream.)
Fixed (comments removed... ;-( ).
> usr/src/lib/libast/Makefile.libastl10n
>
> Line 35 - Same comment as above about the external bug
> reference.
Fixed.
> usr/src/lib/libast/mapfile-vers
>
> Lines 29-618 - Please just include the results and don't embed
> the script inside the comment.
Fixed (I wish we could keep the comments to document how this stuff was
created... ;-( ).
> usr/src/lib/libast/sparc/Makefile
>
> Line 31 - Same comment as above about using only the minor part
> of the release value.
See above.
> usr/src/lib/libast/sparcv9/Makefile
>
> Line 31 - Same comment as above about using only the minor part
> of the release value.
See above.
> usr/src/lib/libc/amd64/Makefile
>
> Line 22 - Update copyright year.
Fixed.
> usr/src/lib/libcmd/common/llib-lcmd
>
> Lines 21-22 - These should appear after the CDDL block,
> copyright and ident #pragma.
Fixed.
> Lines 35-53 - Please just include the results and don't embed
> the script inside the comment.
Fixed.
> usr/src/lib/libcmd/common/Makefile
>
> Is this really the Makefile you intend to putback? It looks
> like it came from the ksh93 source itself.
See above, we've not going to strip the upstream sources.
> usr/src/lib/libcmd/common/mapfile-vers
>
> This is showing up only under "Old". Are you removing it?
"mapfile-vers" should be in the libraries's base directory to avoid that
this may get lost during source updates. The common/ subdir should for
the upstream sources only (only exception is the additions to the demo
code and test suite (e.g. the test to watch over the getconf
compatibilty (usr/demo/ksh/tests/sun_solaris_getconf.sh)) ).
> usr/src/lib/libcmd/Makefile.com
>
> Line 111 - Same comment as above concerning fitting the line
> within 80 columns using string concatenation.
Fixed.
>
> Line 120 - There's no need to include the actual warning that
> you're preventing here.
Fixed.
> usr/src/lib/libcmd/mapfile-vers
>
> Lines 31-39 - Please just include the results and don't embed
> the script inside the comment.
Fixed... ;-(
> Also, it's unclear if this is being renamed from
> usr/src/lib/libcmd/common/mapfile-vers or if you're creating a
> new mapfile-ver (and removing the old one.) You should be
> renaming the file and updating it (which you might be doing as
> I realize you don't have Teamware.)
The generation script which builds the webrev doesn't have support for
moved files and AFAIK moving the tree between the various prototype
versions has destroyed the original movement. Uhm... AFAIK the only
person who can "move" the files in the final putback workspace is
April...
> usr/src/lib/libc/port/regex/wordexp.c
>
> Line 22 - Update copyright year.
Fixed.
General note on the wordexp.c diff: The new version of the workexp() is
a cloned version of the old code and then modified to work with ksh93
and therefore has cloned all the cstyle and other coding layout bits
from the original. The idea of copying the whole function was to avoid
having a zillion of #ifdef/#else/#endif in there which would make the
code more or less unreadable or normal human beings.
/opt/onbld/bin/cstyle however doesn't complain about the current vesion.
[snip (moving this to a seperate email)]
> usr/src/lib/libdll/amd64/Makefile
>
> Line 31 - Same comment as above about using only the minor part
> of the release value.
See above.
> usr/src/lib/libdll/common/Makefile
>
> Is this really the Makefile you intend to putback? It looks
> like it came from the ksh93 source itself.
Yes, see above (no fork() of AST codebase, please).
> usr/src/lib/libdll/i386/Makefile
>
> Line 31 - Same comment as above about using only the minor part
> of the release value.
See above.
> usr/src/lib/libdll/sparc/Makefile
>
> Line 31 - Same comment as above about using only the minor part
> of the release value.
See above.
> usr/src/lib/libdll/sparcv9/Makefile
>
> Line 31 - Same comment as above about using only the minor part
> of the release value.
See above.
> usr/src/lib/libpp/common/llib-lpp
>
> Lines 35-52 - Please just include the results and don't embed
> the script inside the comment.
Fixed.
> However, if this lint library is not going to be delivered (I
> believe it should not until the interfaces are upgraded), then
> it's not clear why you're delivering this at this time.
See comment above for the other lint libraries.
> usr/src/lib/libpp/common/Makefile
>
> Is this really the Makefile you intend to putback? It looks
> like it came from the ksh93 source itself.
Yes, see above (no fork() of AST codebase, please).
> usr/src/lib/libpp/Makefile.com
>
> Line 91 - Same comment as above concerning fitting the line
> within 80 columns using string concatenation.
Fixed.
> Lines 101-113 - There's no need to include the actual warning
> that you're preventing here.
Fixed (comments removed... ;-( ).
> usr/src/lib/libpp/mapfile-vers
>
> Line 29 - I don't think this comment is really necessary
> (almost all of the other mapfiles are also generated by hand.)
Erm, all other AST mapfile-vers stuff is created more or less
automatically and then verified by hand (exceptions include libpp and
libshell (where I had to export all |b_()| functions to avoid a
screw-up)).
> usr/src/lib/libshell/common/data/solaris_cmdlist.h
>
> Line 26 - The file is missing a #pragma ident.
Uhm... this file "pretends" to be an AST source (well, it's added by the
"ksh93_solaris_builtin_patch.diff") and AFAIK doesn't need a "#pragma
ident" (AFAIK this is some kind of border case... ;-/ ).
> Lines 51-66 - Please just include the results and don't embed
> the script inside the comment.
Fixed.
> usr/src/lib/libshell/common/llib-lshell
>
> Lines 21-22 - These should appear after the CDDL block,
> copyright and ident #pragma.
Fixed.
> Lines 33-57 - Please just include the results and don't embed
> the script inside the comment.
Fixed... ;-(
> usr/src/lib/libshell/common/Makefile
>
> Is this really the Makefile you intend to putback? It looks
> like it came from the ksh93 source itself.
See above (unmodified upstream sources).
> usr/src/lib/libshell/Makefile.com
>
> Line 170 - Same comment as above concerning fitting the line
> within 80 columns using string concatenation.
Fixed.
> usr/src/lib/libshell/mapfile-vers
>
> Lines 30-51 - Please just include the results and don't embed
> the script inside the comment.
>
> usr/src/lib/libshell/misc/buildksh93.ksh
> usr/src/lib/libshell/misc/buildksh93.readme
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Did you read the README and the **WARNINGS** in the script itself ?
"buildksh93.ksh" is the way how we build the external includes and is
highly specific for the putback. The script is even modfied for each of
the updates we did during during development and the script is crafted
specifically for that version of the ast-ksh.YYYY-MM-DD.tgz package. If
there is anything we can do really wrong than it's:
1. Loose this script. At that point we loose our abilty to update the
codebase in OS/Net
2. Use the wrong version of the script on the wrong AST sources - that's
a wonderfull method to invite bugs to creep into the headers and
codebase.
Therefore *NO* (and please no bickering about this part - much testing
went into "buildksh93.ksh" and loosing it would more or less FATAL for
this project). At least this script should really stay here to be in
sync with the putback (I am willing to debate the case about the *.diff
files but "buildksh93.ksh" is really off-limits for "place it
elsewhere"-discussions).
> usr/src/lib/libshell/misc/ksh93_solaris_builtin_patch.diff
> usr/src/lib/libshell/misc/ksh93_solaris_builtin_patch.readme
>
> I don't believe these files should be putback to the ON gate.
> It makes sense to keep them on a project-specific website.
Umpf...
... this is per-putback information. It is going to change per putback.
How should an external project side track this and all branches made
from the main OS/Net tree ? And there is no gurantee that the project
webpage will be available in ten or more years which will generate a
serious problem for the maintainers. We already had that problem for
dtksh and except the generic "we try to avoid that mistake in the future
for OpenSolaris" noone has proposed a working alternative which tracks
the progress of ksh93 in OS/Net including all branches.
> usr/src/pkgdefs/SUNW0on/prototype_com
>
> Lines 81-84 - Please sort the list.
Fixed.
> usr/src/pkgdefs/SUNWarc/prototype_com
> usr/src/pkgdefs/SUNWarc/prototype_i386
> usr/src/pkgdefs/SUNWarc/prototype_sparc
>
> As these libraries are currently Project Private, delivering
> them in SUNWarc is inappropriate.
Fixed (moved to the exception lists).
> usr/src/pkgdefs/SUNWastdev/prototype_com
>
> Lines 47-55 - Please sort the list.
Fixed.
> usr/src/pkgdefs/SUNWcsl/prototype_com
>
> Line 239 - Please move this entry up one (to maintain the
> sorted list.)
Fixed.
> sr/src/pkgdefs/SUNWcsl/prototype_i386
>
> Line 297 - Please move this entry up one (to maintain the
> sorted list.)
Fixed.
> usr/src/pkgdefs/SUNWcsl/prototype_sparc
>
> Line 285 - Please move this entry up one (to maintain the
> sorted list.)
Fixed.
> usr/src/pkgdefs/SUNWcsr/prototype_com
>
> Line 223 - Please sort this entry (right after line 184?)
Fixed.
> usr/src/pkgdefs/SUNWcsu/prototype_sparc
>
> Lines 50-51 - What is the reason for shipping a 32-bit version
> on SPARC? Can ksh93 be used to read 32-bit core files or
> processes? :-)
There are several reasons including:
- ksh93 supports (loadable) binary plugins which may itself rely on
other shared libraries which may not be available as 64bit versions. In
those cases a 32bit ksh93 is needed. I am waiting for a sponsor for
http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6474270
("isaexec and magical builds") (see request-sponsor queue) to make
"isaexec" adjustable for applications which need such a functionality
(via restricting the list of ISAs based on accept and/or reject filter
environment variables).
- We need libshell&co. around for future consumers like the various
tools currently wrapped in "alias.sh" (this includes things like
/usr/bin/test etc.), "shcomp" (shell script compiler), /usr/bin/sleep
(using a 64bit binary for this is IMO an overkill), /usr/bin/printf etc.
and obmitting the 32bit shell would not be wise in this case (for
example: how else can we test the libraries then ?).
> usr/src/Targetdirs
>
> Lines 215-216 - Please sort these entries (right after line
> 212?)
Fixed.
----
Bye,
Roland
--
__ . . __
(o.\ \/ /.o) roland.mainz at nrubsig.org
\__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer
/O /==\ O\ TEL +49 641 7950090
(;O/ \/ \O;)
