April Chin writes:
> The webrev for all of the other (non-AST) files, including Makefile
> and packaging changes, is at:
>
> http://cr.opensolaris.org/~chin/ksh93-update1.non-ast
By request, I looked at only makefiles usr/src/cmd and usr/src/lib.
(In a few cases, I veered off that path to sample some of the llib and
mapfile changes.)
Some general issues:
It can be helpful, especially for huge sets of changes like this, to
use the "-w" option in webrev to assign particular comments for
individual files. Obviously, Mercurial doesn't like to track things
that way in the actual commit logs, but doing this with webrev can
help out reviewers who are trying to match up CRs and ARC cases
against actual code changes.
If there are no substantive changes in a file, then the file
shouldn't be part of the review nor part of the eventual
integration. In particular, there's no reason to rototill the CDDL
and copyright text alone in any file.
The libraries look like they need some additional TLC. I'm guessing
that *might* be something on a future development path, but if not,
then I'll flag some of those issues here.
usr/src/Makefile.ast
38-39: I'm not certain I know what these new directives are for, but
it might be nice to add "install_h" targets for these
directories so that the header files get installed in the
proto area, and thus you won't need reach-around "-I"
directives. You don't have to place everything that's in the
proto area into a package for delivery; that's what the
exception lists are for.
(As a matter of general practice in ON, that's what we do: we
put our private header files into $ROOT/usr/include along
with all the rest, and then use the packaging definitions to
determine which things actually get delivered and which are
just used as part of the build alone.)
(If the header files in those special directories somehow
conflict with or corrupt something else, and thus have to be
hidden away out of $ROOT, then ignore this comment.)
67: I'm not sure I know the importance of this change. April (or
someone else associated with the project) should look into the
legal issues to make sure that this name change doesn't cause
trouble.
usr/src/Makefile.ksh93switch
There are no substantive changes here. Is this part of the review?
usr/src/cmd/Makefile
388,699: it looks like you've deleted "sum" from this list, but have
not deleted the contents of the usr/src/cmd/sum/ directory.
usr/src/cmd/ast/Makefile
Reviewed; no comments.
usr/src/cmd/ast/msgcc/Makefile
48: is this really needed? (Do you have Japanese source code?) If
it is needed, then a comment about the extra options used would
be nice to have; perhaps right before line 46.
54: out of curiosity, what stops it from using shcomp now? (I
assume it's that you can't assume the build machine has it,
because this project is actually what installs shcomp, and you
don't want to build a native copy just for the build process.
Right?)
usr/src/cmd/ksh/Makefile
32:nit: 'pfrksh' seems like an odd combination of beasts to me.
67-77: I still wish these were done with normal make targets like
everything else in ON where we build symlinks, rather than as
in-line shell scripts. I know we've been over this ground
before, but using extensive shell scripting inside makefiles
is just plain ugly to me, particularly when make works well
without it. And, no, I don't care a whit about "performance"
for a trivial loop that only runs two or at most 6 times per
build.
usr/src/cmd/ksh/Makefile.com
31-35: why do these lists need to be copied in both the top level
makefile and here? It'd be nice to have them in exactly one
place. Perhaps the top-level makefile
(usr/src/cmd/ksh/Makefile) should itself just include this
Makefile.com.
61,65: more character set mystery; need at least a comment.
usr/src/cmd/ksh/Makefile.testshell
95-163: for anything over a line or two, I strongly suggest moving
it into a separate shell script that then is invoked by the
makefile as needed. This style (with embedded scirpts) is
far less readable, particularly because of the quoting
requirements of makefiles. Too much of this looks like line
noise.
usr/src/cmd/ksh/amd64/Makefile
35-44: apparently duplicated logic from the top-level makefile;
something is odd here. (I'd expect either every
per-architecture makefile to install the symlinks *OR* the
top-level makefile to do it, but why should both be attacking
the problem?)
usr/src/cmd/ksh/builtins/Makefile
64-67: there's an existing $(INS.link) that does this; please use
it.
usr/src/cmd/ksh/i386/Makefile
usr/src/cmd/ksh/sparc/Makefile
usr/src/cmd/ksh/sparcv9/Makefile
35-43: as above, unexpectedly duplicated logic.
usr/src/cmd/shcomp/Makefile
82-87: if you're not using *.po files, then why is this needed?
Things not using them are usually just skipped over for the
msgs build -- and if it is needed, then whatever is
descending in here and trying to build or use *.po files
should be stopped.
usr/src/lib/Makefile
Reviewed; no comments.
usr/src/lib/Makefile.asthdr.
#include <std/comment-regarding/embedded-scripts.h>
usr/src/lib/Makefile.astmsg
50-51: yay!
usr/src/lib/libast/Makefile
usr/src/lib/libast/Makefile.com
usr/src/lib/libast/amd64/Makefile
Reviewed; no comments.
usr/src/lib/libast/common/llib-last
129-945: this stuff doesn't belong here; properly-constructed 'llib'
files should only need #includes -- the same #includes that
the software linting against the library would itself have
to use in order to get the interface definitions. Having
giant lists of externs is a sign of internal design
trouble. At a minimum, it means that the #includes are
wrong.
usr/src/lib/libast/i386/Makefile
usr/src/lib/libast/sparc/Makefile
usr/src/lib/libast/sparcv9/Makefile
usr/src/lib/libcmd/Makefile
Reviewed; no comments.
usr/src/lib/libcmd/Makefile.com
128,132: mystery flag issue.
usr/src/lib/libcmd/amd64/Makefile
There are no substantive changes here; is this part of this
integration?
usr/src/lib/libcmd/common/llib-lcmd
35-90: more design-level trouble.
usr/src/lib/libcmd/i386/Makefile
usr/src/lib/libcmd/sparc/Makefile
usr/src/lib/libcmd/sparcv9/Makefile
No substantive changes.
usr/src/lib/libdll/Makefile
Reviewed; no comments.
usr/src/lib/libdll/Makefile.com
81,85: mystery flags.
usr/src/lib/libdll/amd64/Makefile
usr/src/lib/libdll/common/llib-ldll
usr/src/lib/libdll/i386/Makefile
usr/src/lib/libdll/mapfile-vers
usr/src/lib/libdll/sparc/Makefile
usr/src/lib/libdll/sparcv9/Makefile
No substantive changes.
usr/src/lib/libpp/Makefile
Reviewed; no comments.
usr/src/lib/libpp/Makefile.com
97,101: mystery flags.
usr/src/lib/libpp/common/llib-lpp
usr/src/lib/libpp/i386/Makefile
usr/src/lib/libpp/mapfile-vers
usr/src/lib/libpp/sparc/Makefile
No substantive changes.
usr/src/lib/libshell/Makefile
Reviewed; no comments.
usr/src/lib/libshell/Makefile.com
153,157: mystery flags.
160-161: what happened to require these new overrides?
usr/src/lib/libshell/Makefile.demo
143: what's this .WAIT do? It doesn't appear to be necessary.
usr/src/lib/libshell/amd64/Makefile
No substantive changes.
usr/src/lib/libshell/common/llib-lshell
35-143: design trouble.
usr/src/lib/libshell/i386/Makefile
No substantive changes.
usr/src/lib/libshell/sparc/Makefile
usr/src/lib/libshell/sparcv9/Makefile
usr/src/lib/libsum/Makefile
Reviewed; no comments.
usr/src/lib/libsum/Makefile.com
73,77: ?
94-95: please remove. (Or at least get rid of the useless $(TRUE).)
usr/src/lib/libsum/THIRDPARTYLICENSE
Not an issue for my review, but someone should make sure that this
new license gets packaged and delivered properly.
usr/src/lib/libsum/THIRDPARTYLICENSE.descrip
This description doesn't seem to match the license itself. The
license itself seems to be "CPL" with IBM as a "steward" (serving
soft drinks and peanuts, I guess). This description says AT&T,
which appears nowhere in the license itself. What gives?
usr/src/lib/libsum/amd64/Makefile
Reviewed; no comments.
usr/src/lib/libsum/common/llib-lsum
34-43: these lines should not be needed if the #include file set is
correct. Is the #include here correct and complete? If not,
can you fix it?
usr/src/lib/libsum/i386/Makefile
usr/src/lib/libsum/mapfile-vers
usr/src/lib/libsum/sparc/Makefile
usr/src/lib/libsum/sparcv9/Makefile
Reviewed; no comments.
--
James Carlson, Solaris Networking <james.d.carlson at sun.com>
Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
