Roland, > 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. 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, unless some compelling reason can be shown where a ksh (or ksh93) construct greatly simplies things. 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. 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. 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? And in those cases, have you added the appropriate Copyright to each of those files? 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. 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 \ usr/src/cmd/ksh/i386/Makefile Line 37 - I don't understand the comment regarding /sbin here. Line 43 - Same comment as above concerning the "for" style. usr/src/cmd/ksh/Makefile Line 70 - Same comment as above concerning the "for" style. 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 but assuming it does, why not on the i386/amd64 side as well? 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. usr/src/cmd/ksh/Makefile.testshell Line 43 - Same comment as above concerning the "for" style. usr/src/cmd/ksh/sparc/Makefile Line 37 - I don't understand the comment regarding /sbin here. Line 43 - Same comment as above concerning the "for" style. usr/src/cmd/ksh/sparcv9/Makefile Line 43 - Same comment as above concerning the "for" style. 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? usr/src/lib/libast/common/llib-last Lines 36-140 - Please just include the results and don't embed the script inside the comment. 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. usr/src/lib/libast/i386/Makefile Line 31 - Same comment as above about using only the minor part of the release value. 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.) usr/src/lib/libast/Makefile.astinclude Line 41 - s/am Intel/an Intel/ Lines 54, 57 - s/Solaris/OpenSolaris/ 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.) usr/src/lib/libast/Makefile.libastl10n Line 35 - Same comment as above about the external bug reference. usr/src/lib/libast/mapfile-vers Lines 29-618 - Please just include the results and don't embed the script inside the comment. usr/src/lib/libast/sparc/Makefile Line 31 - Same comment as above about using only the minor part of the release value. usr/src/lib/libast/sparcv9/Makefile Line 31 - Same comment as above about using only the minor part of the release value. usr/src/lib/libc/amd64/Makefile Line 22 - Update copyright year. usr/src/lib/libcmd/common/llib-lcmd Lines 21-22 - These should appear after the CDDL block, copyright and ident #pragma. Lines 35-53 - Please just include the results and don't embed the script inside the comment. 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. usr/src/lib/libcmd/common/mapfile-vers This is showing up only under "Old". Are you removing it? usr/src/lib/libcmd/Makefile.com Line 111 - Same comment as above concerning fitting the line within 80 columns using string concatenation. Line 120 - There's no need to include the actual warning that you're preventing here. usr/src/lib/libcmd/mapfile-vers Lines 31-39 - Please just include the results and don't embed the script inside the comment. 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.) usr/src/lib/libc/port/regex/wordexp.c Line 22 - Update copyright year. Lines 89-90 - Cstyle error - Please put the while on the same line as the closing curly brace } while (*s++ != '\0'); Line 117 - s/neccessary/necessary/ Line 135 - There seem to be multiple spaces between the words in several parts of this line. Line 144 - s/allocting/Allocate/ Line 151 - Cstyle error - continuation lines should be indented four spaces. Lines 217-226 - Could you provide more details here? Is /usr/lib/libc/libc_wordexp_commands another interface that can be customized by users? Lines 308, 310, 312 - Is there any header file which #defines these ksh93 exit codes? usr/src/lib/libdll/amd64/Makefile Line 31 - Same comment as above about using only the minor part of the release value. 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. usr/src/lib/libdll/i386/Makefile Line 31 - Same comment as above about using only the minor part of the release value. usr/src/lib/libdll/sparc/Makefile Line 31 - Same comment as above about using only the minor part of the release value. usr/src/lib/libdll/sparcv9/Makefile Line 31 - Same comment as above about using only the minor part of the release value. usr/src/lib/libpp/common/llib-lpp Lines 35-52 - Please just include the results and don't embed the script inside the comment. 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. 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. usr/src/lib/libpp/Makefile.com Line 91 - Same comment as above concerning fitting the line within 80 columns using string concatenation. Lines 101-113 - There's no need to include the actual warning that you're preventing here. 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.) usr/src/lib/libshell/common/data/solaris_cmdlist.h Line 26 - The file is missing a #pragma ident. Lines 51-66 - Please just include the results and don't embed the script inside the comment. usr/src/lib/libshell/common/llib-lshell Lines 21-22 - These should appear after the CDDL block, copyright and ident #pragma. Lines 33-57 - Please just include the results and don't embed the script inside the comment. 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. usr/src/lib/libshell/Makefile.com Line 170 - Same comment as above concerning fitting the line within 80 columns using string concatenation. 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 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. usr/src/pkgdefs/SUNW0on/prototype_com Lines 81-84 - Please sort the list. 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. usr/src/pkgdefs/SUNWastdev/prototype_com Lines 47-55 - Please sort the list. usr/src/pkgdefs/SUNWcsl/prototype_com Line 239 - Please move this entry up one (to maintain the sorted list.) sr/src/pkgdefs/SUNWcsl/prototype_i386 Line 297 - Please move this entry up one (to maintain the sorted list.) usr/src/pkgdefs/SUNWcsl/prototype_sparc Line 285 - Please move this entry up one (to maintain the sorted list.) usr/src/pkgdefs/SUNWcsr/prototype_com Line 223 - Please sort this entry (right after line 184?) 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? :-) usr/src/Targetdirs Lines 215-216 - Please sort these entries (right after line 212?) dsc
