Mike Kupfer wrote: > I looked at the makefiles for Roland's changes, using > > svn diff -r 178:287 > http://svn.genunix.org/repos/on/branches/ksh93/gisburn/prototype002/m1_ast_ast_imported > > I've got a bunch of comments and things I'd like to see changed, but > before I get to that, I want to thank Roland for (a) doing a huge pile > of work, and (b) being patient with me (I know I haven't been responding > as quickly as anyone would like). > > Also, I should mention that I'll be away from email starting sometime > tomorrow (2006-06-23) through sometime in the middle of next week. > > Anyway... > > Overall, I think it's in pretty good shape. Some specific issues: > > * cmd/ksh/Makefile.com > - why is GROUP set to root?
Because I cloned the Makefile from another application (AFAIK "newtask") which used "isaexec" and I didn't change that detail... ;-( > The current Solaris ksh88 is installed as > group bin. Fixed (see http://polaris.blastwave.org/changeset/294). > * usr/src/cmd/ksh/Makefile.ksh93switch > - for putback, BUILD_KSH93_AS_BINKSH will need to be 0. Known problem... I am using it with this configuration for testing right now (the other nit is that the new version |libc::wordexp()| spamms the syslog with it's messages... that debug code needs to disabled, too). > * usr/src/cmd/ksh/Makefile.com > usr/src/lib/libshell/Makefile.com > - CPPFLAGS: it would better to have a single makefile fragment for > -D/-U that is included by these two makefiles I thought about that - but the usr/src/cmd/ksh/Makefile.com AFAIK differes in the -I statements used since it does not have to reference libshell's internal sources. Your choice: Should I add a common "Makefile.libshellflags" for these options or leave it in it's current status ? > * usr/src/cmd/ksh/Makefile.com > - install target: why add the "set -x"? >From ksh(1): -- snip -- -x Prints commands and their arguments as they are executed. -- snip -- The basic idea is to see which exact command failed when a problem occurs. Otherwise it may be required to look at the Makefile, trying to figure out how it works (or fails), how it is supposed to work and then the original author of that piece of code gets cursed for adding such a "hard to debug" monstrosity. Having the shell's "xtrace" flag "on" by default AFAIK doesn't harm anyone and is IMO helpfull when something breaks... :-) > * usr/src/cmd/ksh/Makefile > - where are we with i18n? will we have a real ksh93.po prior to > putback? I think you're asking here about l10n (=localisation), right ? i18n support is working (see http://www.opensolaris.org/os/project/ksh93-integration/screenshots/ksh93-i18n_001.png) thanks to the patches applied for ksh93r+ (at least Ienup Sung seems to be happy with it). Localisation (AFAIK the *.po stuff is for l10n) is working in the ksh93 binaries - but I simply didn't investigate the details including: - Where are the localisation files used for the different locales stored ? In OS/Net or elsewhere ? If they are stored elsewhere - how can we keep track of changes (syncronisation issue) ? - AST has a number of localisation available, however their encoding (e.g. single-byte vs. UTF-8 vs. other multibyte encodings) is unclear and needs more investigation together with someone who really knows how Solaris handles this stuff - The libast API is AFAIK different from the normal Solaris libc API for that (David/Glenn, please correct me if I am wrong here) so the *.po stuff will likely not be used. - Only a microscopic fraction of languages+encodings supported by Solaris is currently provided by upstream. We really need more translators... > * usr/src/lib/libshell/common/Makefile > usr/src/lib/libdll/common/Makefile > usr/src/lib/libast/common/Makefile > - I think that having these files in the ON tree will cause confusion. > I'd like either to remove them or explain in README's that this file > is not used for ON. (Maybe use > lib/<library>/common/README.opensolaris?) Added to my ToDo list (killing them is an option, but the next diff between ksh93 and Solairs sources then list these files as "missing" and the person who does the update then has figure out the "why ?". And these Makefiles provide some interesting (well, not essential) information, too - so I vote to not delete them (they also have no CDDL header, making them easy to identify as non-Solaris source code)). I'll write a usr/src/lib/libshell/README.OpenSolaris (and usr/src/lib/libdll/README.OpenSolaris, usr/src/lib/libcmd/README.OpenSolaris, usr/src/lib/libast/README.OpenSolaris and usr/src/cmd/ksh/README.OpenSolaris pointing to that file) which describes these and other issues (including how the ksh93/libshell/libdll/libcmd/libast-automatically generated files can be updated). > * usr/src/lib/libshell/common/Makefile.com > usr/src/lib/libcmd/Makefile.com > usr/src/cmd/ksh/Makefile.com > usr/src/lib/libdll/Makefile.com > usr/src/lib/libast/Makefile.com > - CPPFLAGS: what breaks when you put /usr/include in front of the > include list? Several other ON components deal with this by using > > CPPFLAGS = <stuff> $(CPPFLAGS.master) > > If that would work here, I think we should do it--it should help > maintainability of the ON code as a whole. The problem is that libast has headers which are meant to "replace" the normal system headers, e.g. /usr/include/ast/stdio.h which is included as <stdio.h> in the various AST sources. Since $(CPPFLAGS.master) includes stuff "-I$(ROOT)/usr/include" and normally comes before any other -I options from the Makefile* it breaks this. Using $(CPPFLAGS.master) is tricky, too - it MUST come as the last element but future changes in -D may then cause silent breakage in the AST sources because the last -D specified overrides previous -D options so I prefer the current way to expliclity list each single flag. Otherwise it may end-up in a horrible pain to figure out a problem which is simply caused by a tiny change in the flags used in $(CPPFLAGS.master). What should I do here now - leave it as it implemented currently or try to "squish" $(CPPFLAGS.master) somewhere into the chain in the hope that this won't bite back in the future... ? > * usr/src/lib/libshell/Makefile.com > usr/src/lib/libcmd/Makefile.com > usr/src/lib/libast/Makefile.com > - I see that the workaround hack for the PAGESIZE redefinition is > still in place. This will need to be changed prior to putback. (At > least I assume that the "CERRWARN += -erroff=E_MACRO_REDEFINED" in > libast/Makefile.com is for PAGESIZE and nothing else.) Yes... the problem here is that the PAGESIZE stuff sits in the automatically generated AST headers (there are two other locations in the source which have a similar problem and can easily be "silenced" using the same solution (however until now I didn't apply _ANY_ changes to the original AST sources)). Adding a workaround (e.g. |#undef PAGESIZE|) is possible but the next update of these headers will kill it - and then we have a build failure which needs to be investigated, e.g. making this a pain regardless of which way we choose. > * usr/src/lib/libshell/Makefile > usr/src/lib/libdll/Makefile > usr/src/lib/libast/Makefile > - these still have the comment "64bit disabled for now due lack of > AMD64 build machine". I thought the issue was that the 64-bit > support is broken, and that it's being fixed upstream. Originally I disabled the 64bit support until I had time to seize a AMD64 box to work on that problem. During that work I hit the problem that the public includes of libast and libshell differ between 32bit and 64bit builds. The same problem exists on SPARC64, too. There are two solutions: 1. Ship two sets of includes, one for 32bit and one for 64bit (for example stored at /usr/include/64/), however I am not sure whether this is worth the trouble (for example backwards-compatiblity (Okok, the API isn't open yet but the /usr/include/64/ subdir may attract other people to do the same instead of delivering proper includes)). 2. Wait until AST upstream fixed the problem I like to wait for [2] (and commited a comment change as http://polaris.blastwave.org/changeset/296) unless you or April think it's neccesary to do [1] ... > * usr/src/lib/Makefile > - you shouldn't need the new .WAIT's (for libast et al), since you > have the explicit dependencies listed further down in the makefile Fixed (see http://polaris.blastwave.org/changeset/295). > * usr/src/lib/libdll/sparc/Makefile > usr/src/lib/libdll/i386/Makefile > usr/src/lib/libast/sparc/Makefile > usr/src/lib/libast/i386/Makefile > - HOSTTYPE still has the OS release number hardwired in. If this > won't actually cause problems for future releases (when uname -r > moves to 5.12), then we can treat this as a cleanup nit, I think. It won't cause problems directly but I added that to my ToDo list to get it fixed somehow prior to the first putback or shortly after that point... > * usr/src/lib/libast/i386/Makefile > usr/src/lib/libast/sparc/Makefile > usr/src/lib/libast/Makefile.com > - these all have code to enable PIC; does that really need to be > in all 3 places? AFAIK no, I hope the "fix" in http://polaris.blastwave.org/changeset/297 is the correct solution... ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;)
