[ksh93-integration-discuss] Re: [osol-code] Round two: ((pre-)pre-review) ksh93-integration webrev 2007-02-02
David.Comay at Sun.COM
David.Comay at Sun.COM
Mon Feb 5 23:46:56 PST 2007
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
More information about the ksh93-integration-discuss
mailing list