[ksh93-integration-discuss] final ksh93 integration code review

Tue Jul 3 19:46:20 PDT 2007

 > 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