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

[Peter Memishian]

Roland, apologies for the delay.  I think we're rapidly converging.

 > > [ Resolved issues removed. ]
 > > 
 > >  > >  > http://cr.grommit.com/~chin/ksh93-webrev-jun29/jun29-makefiles/
 > 
 > I still feel uneasy about the idea to create one seperate file for the
 > C99MODE+CERRWARN stuff because it's AFAIK not a "catch all" solution.
 > Some items in the Makefiles seem to require a specific position (e.g.
 > before or after "include */Makefile.lib" etc.) and somehow I feel we may
 > end up with something like usr/src/Makefile.ast_pre_stuff,
 > usr/src/Makefile.ast_main_stuff and usr/src/Makefile.ast_post_stuff
 > (just a feeling) ...
 > ... and the KSHCPPFLAGS idea sounds nice but I'd like to wait for
 > "shcomp" (ksh93's shell script bytecode compiler) to land in the tree

As per your follow-up mail, I know you went ahead with this -- but did you
also do the KSHCPPFLAGS bit?  That part's especially important to me
because without it we may have drift between the two definitions, and I
don't see the particular need to wait for "shcomp".

 > Ok...
 > ... I've moved Makefile.ksh93switch to usr/src/ to avoid the reaching
 > over" to other "peer" directories...

Cool.

 > >  > > lib/libcmd/Makefile.com:
 > >  > >
 > >  > > * 104: "-Isrc/lib/libcmd" seems bizarre; why is this necessary?
 > > 
 > > I'm not sure I understand this yet.
 > 
 > Ok... lets describe this differently. The "src/lib/libcmd" part in
 > "-Isrc/lib/libcmd" has nothing to do with the OS/Net source layout, it's
 > the relative subpath in usr/src/lib/libcmd/$(TRANSMACH)/, e.g.
 > usr/src/lib/libcmd/$(TRANSMACH)/src/lib/libcmd/ ... and the same applies
 > to the other AST libraries, for example libast has
 > usr/src/lib/libast/$(TRANSMACH)/src/lib/libast/. This is how the
 > upstream directory layout looks like (see
 > http://mail.opensolaris.org/pipermail/busybox-dev/2007-April/000033.html
 > , note that the "src/lib/lib(ast|dll|pp|cmd|shell)/"-part cannot be
 > changed without poking in the AST sources (see below (the comment about
 > maintaince))).

I get it now, thanks :-)  Yes, a bit alien, but I understand why you've
chosen to do it that way.

 > >  > > 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.)
 > Yes, but I can "predict" the results of the changes of my changes to
 > "Makefile.lib". Adding new variables with VERY generic names (BTW: I
 > wish "RELEASE" would be renamed to "OSNET_RELEASE") in "Makefile.master"
 > is IMO a different thing because I don't know whether this may trigger
 > problems if someone (for example in closed_bins/ or elsewhere) uses the
 > same variable names (and I don't have the CPU power or disk space to
 > setup a couple of "playground" trees and run a few tests (remeber my
 > machines chew two days on OS/Net and the Ultra5 was originally shipped
 > with 9GB disks... ;-( )).

I checked; no one in usr/closed is making use of RELEASE_MAJOR or
RELEASE_MINOR.  I proposed generic names because they are generic
macros :-)

 > BTW: See
 > http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/ipf/Makefile.ipf#17

That's a shame.

Anyway, like I said before, if you want to delay splitting apart RELEASE
into RELEASE_MAJOR and RELEASE_MINOR, I'm OK with that.  But a bug should
be filed on the issue so that we can clean up the ksh93 Makefiles and
other victims of the current Makefile.master logic in the future.

 > >  > >           39-46 of cmd/ksh/i386/Makefile.
 > >  >
 > >  > This logic is needed to install isaexec links for all variants of ksh
 > >  > (e.g. restricted shell, profile shell etc.) depending on the setting in
 > >  > Makefile.ksh93switch and the values of USRKSH_ALIAS_LIST and PROG,
 > >  > ...
 > > 
 > > I think I understand what the logic is for.  What I don't understand is
 > > why the *same* logic appears to exist in cmd/ksh/Makefile,
 > > cmd/ksh/i386/Makefile, and cmd/ksh/sparc/Makefile.
 > 
 > The logic in usr/src/cmd/ksh/Makefile is for the isaexec hardlinks while
 > the logic in usr/src/cmd/ksh/$(TRANSMACH)/ is for the binary and it's
 > hardlinks itself (and they can't move to Makefile.com since there is a
 > difference between ROOTPROG32 and ROOTPROG64 (and the detail that the
 > Makefiles in usr/src/cmd/ do not define TRANSMACH (that's why I added
 > CMDTRANSMACH))).

I see, thanks :-)

 > >  > >         * 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.)
 > >  >
 > >  > Erm... I don't have a build machine > B61 and I can't simply update my
 > >  > two build machines because I would either loose the abilty to test the
 > >  > |libc::wordexp()| code or have to update the whole tree to a newer
 > >  > Nevada build (which would trigger problems for April because she would
 > >  > have to re-do her whole SCCS tree...).
 > > 
 > > But you're well aware of the issue, so the comment is not for you, right?
 > > Others will be running on newer builds (since B61 is history), so I don't
 > > see why the comment is needed.
 > 
 > Erm, the comment is needed because the shell code below adds an
 > exception for the matching test run. I'd like to explain _why_ the
 > exception is currently there (and AFAIK there is no variable to test for
 > the Nevada build version (e.g. B[0-9]*)) to make the exception
 > conditional... ;-(

But why do you need to putback the exception?  Build 61 and earlier are
history, and our source should not discuss issues that no longer apply.

 > >  > >         * 113-143: Please reformat to be 80-column friendly.
 >
 > Well, there is a comment about rewriting the whole monstrosity in ksh93.
 > At least all the "egrep" and "grep" things will disappear and the whole
 > thing will become more compact and easier to maintain. A seperate script
 > is slightly tricky since it depends on _lots_ of variables in the
 > Makefiles which would need to be passed as parameters somehow - and the
 > result won't be much prettier then the current solution... ;-/
 > ... anyway... I've re-arranged the script to fit within the 80-column
 > margin. The comments have been re-arranged to fit within the 80-column
 > margin except the URL (otherwise you can't click it in the source
 > browser) and the output of the "options.sh" test failure...
 > ... is that Ok ?

Yes, that sounds reasonable to me.

One final tiny request: would you be willing to rename Makefile.astinclude
to Makefile.ast or Makefile.astlib?  My concern is that the name is
several characters wider than anything else in usr/src/lib and makes it
harder to see the full set of files that are in the directory with ls(1).
(And AFIACT the "include" part is not particularly meaningful.)

--
meem