> http://cr.grommit.com/~chin/ksh93-webrev-jun29/jun29-makefiles/
I've completed my review of the Makefiles. Things are looking good --
most of my comments are quite minor. Nice work, Roland :-)
General:
* Seems like every AST-related Makefile contains:
# Override this top level flag so the compiler builds in its
# native C99 mode. This has been enabled to support the math
# stuff in the AST tools.
C99MODE= $(C99_ENABLE) -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1
# silence common libast&co. warnings (upstream will handle this
# later) ... ... about |#pragma prototyped| ...
CERRWARN += -erroff=E_UNRECOGNIZED_PRAGMA_IGNORED
It would be really nice if we could centralize these somewhere.
(Just a request, not a requirement.)
* I realize the use of SHELL=/usr/bin/ksh has been discussed
before, and I know you want to keep it. But for the library
Makefiles, can we please consolidate it into libfoo/Makefile.com
and yank it out of libfoo/$ISA/Makefile? (That should also
allow you to remove libcmd/$ISA/Makefile from your list of
changed files.)
* A nit, but almost every libfoo/Makefile.com has "explicitly"
misspelled as "expliclity".
* Similarly another nit: almost every libfoo/Makefile has a
needless "definitions for install_h target" comment above HDRS.
lib/Makefile:
* 515: libshell appears to have a spurious dependency on libnsl.
lib/libc/{sparc,sparcv9,amd64,i386}/Makefile.com:
* It's a shame this has to be duplicated across all four Makefiles,
but I guess that's status quo with the libc Makefiles.
* In theory, you need a FLG here so that when someone brings
over usr/src/lib/libc they'll get Makefile.ksh93switch brought
over too. But FLGs are dying soon with the Mercurial switch,
so I guess it doesn't matter.
lib/libcmd/Makefile:
* 51-52: Not sure why the HDRDIR32/HDRDIR64 handling warrants a
comment here (none of the other uses seem to have comments).
lib/libcmd/Makefile.com:
* 104: "-Isrc/lib/libcmd" seems bizarre; why is this necessary?
* 131-132: Comment about fnamecheck omission seems unnecessary.
lib/libdll/{sparc,sparcv9,amd64,i386}/Makefile:
* GETRELEASEMINOR seems the hard way around; seems simpler to
update Makefile.master to have:
RELEASE_MAJOR=5
RELEASE_MINOR=11
RELEASE=$(RELEASE_MAJOR).$(RELEASE_MINOR)
... then get rid of GETRELEASEMINOR. (But if for some reason it
needs to be kept, then please move it somewhere more common.)
* Seems simpler to replace DLLPLATFORMCPPFLAGS with
DLLPLATFORM=sun4 (or i386), and then have Makefile.com have
"-DHOSTTYPE="sol$(RELEASE_MINOR).$(DLLPLATFORM)" in CPPFLAGS.
lib/libast/{sparc,sparcv9,amd64,i386}/Makefile:
* Same comments as libdll above.
lib/libpp/Makefile.com
* 100: s/execept/except/
lib/libshell/Makefile.demo:
* 74: Continuation line here seems awkward.
lib/libshell/Makefile.com:
* 122: Comment mentions libnsl, but it's not in the corresponding
macro.
* 134-171: Could we keep the CPPFLAGS in cmd/ksh/Makefile.com
and here in-sync through includes instead?
* 143: -Isrc/cmd/ksh93 seems suspicious; why are header files
for the library lurking over in a command directory?
cmd/Makefile:
* 31: Why include ksh/Makefile.ksh93switch here? If we really
need this, seems like Makefile.ksh93switch needs to move to a
more common directory.
cmd/ast/msgcc/Makefile
* 70: Comment about "install rules" doesn't seem to match code
that follows.
* 73: "main" comment here seems unnecessary.
* 80-82: Suggest consolidating into `clean lint:'
* 84-90: These lines should be needless now that all the programs
are listed in $(PROG).
cmd/ksh/Makefile:
* 65-74: I'm confused why this logic is here and at e.g. lines
39-46 of cmd/ksh/i386/Makefile.
cmd/ksh/Makefile.com:
* 39-43: This comment seems a bit hard to believe, since there
seems to be a sizeable difference between this Makefile's
structure and the one in libshell. Given that there's just
a single source file, I don't see the need for mkpicdirs.
* 49: See previous FLG-related comment.
* 52: Extra blank line.
* 57-58: See previous comment on the need to share this
list with libshell.
* 132: I think this comment is supposed to explain why we delete
ksh and ksh93 even for `clean' (rather than `clobber'), but I'm
not quite getting it.
cmd/ksh/Makefile.testshell:
* 38-44: So will this test suite failure be resolved prior to
integration?
* 56-61: Since we're well past build 64, can this issue be
removed? (If not, please reformat the comment to be 80-column
friendly.)
* 66: s/compliciated./complicated./
* 81, 95: Use of csh prompt syntax seems surprising here ;-)
* 100: s/weired/weird/.
* 104: One exclamation point will do ;-)
* 113-143: Please reformat to be 80-column friendly.
cmd/ksh/Makefile.ksh93switch:
* 28-30: Nit: the suggestion would be easier to parse if we
removed the embedded comment -- e.g.:
# Should we build ksh93 as /bin/ksh ?
# This can be overridden at build time via:
# $ export ON_BUILD_KSH93_AS_BINKSH=1
* 37: Extra blank line.
usr/src/lib/Makefile.astmsg:
* 29: Should "Temporary control building" be "Temporary control
over building"? (Or maybe "Temporarily" was meant?)
* 33-35: Similar nit to Makefile.ksh93switch.
* 44: s/mesage/message/
* 60-63: Seems like the common part of these macros should be
moved into ASTMSGCCFLAGS. (I'm also unclear why ASTMSGCCFLAGS
is needed, rather than directly using CPPFLAGS.)
* 69, 63, 76: Why is CFLAGS needed?
* 79-81: Seems like it would be simpler as:
$(ASTMSGCATALOG): $(MSGLIBNAME).msg
@$(RM) $@; \
$(SED) 's/^$$translation msgcc .*//' $(MSGLIBNAME).msg | gencat
$@ -
... but I haven't tested it.
* 90: $(TOUCH) instead?
cmd/sgs/libelf/Makefile.com:
* Not sure why the DIRMODE setting is no longer needed.
Makefile.lint:
cmd/ast/Makefile:
cmd/nsadmin/Makefile:
cmd/sgs/libelf/Makefile.targ:
src/cmd/ksh/{sparc,sparcv9,amd64,i386}/Makefile:
lib/libast/Makefile:
lib/libpp/Makefile:
lib/libdll/Makefile:
lib/libdll/Makefile.com
lib/libcmd/{sparc,sparcv9,amd64,i386}/Makefile:
lib/libpp/{sparc,i386}/Makefile:
lib/libshell/{sparc,sparcv9,amd64,i386}/Makefile:
pkgdefs/Makefile:
pkgdefs/SUNWastdev/Makefile:
* Reviewed, no comments beyond the general ones.
--
meem
