Roland Mainz wrote: > James Carlson wrote: > > Roland Mainz writes: > > > * Webrev over all non-AST files (this includes the files in > > > usr/src/cmd/ast/msgcc/ my accident): > > > http://www.nrubsig.org/people/gisburn/work/solaris/ksh93_integration/ksh93_integration_prototype004_webrev_20070127/non_ast_files/webrev/ > > > > I did some light review of this. Some comments: > > > > cmd/ast/msgcc/Makefile > > > > 56-66: why are these needed? Don't the default targets work > > properly? The usual way to do this would be: > > > > PROG= msgcvt msggen msgget msgcpp > > > > and let make figure out the implicit dependencies. Is there some > > reason that doesn't work here? > > Not really... the Makefile was a patchwork hacked together at 5:00AM > from other bits of the tree (and it seems I collected all the > imagineable bugs, too... ;-( ). > > Note that the ASTPROG was used because I expect that the AST* rules move > to the Makefile fragment which run the normal+XPG[46]* rules some day > (if we get more AST tools in the tree) ... > > > 69-75,78-83: consider creating a common build rule for these. > > Fixed. > > > 91-92: what is this? This ought o be a part of the Targetdirs. > > Ouch... > ... Fixed. > > > 99: I'd suggest a dependency rather than .WAIT. > > Fixed (or better: removed since the directory creation was moved to > Targetdirs). > > > 105-110: why is this needed? What's wrong with the default rules? > > Nothing except that I used the custom rules for debugging. > > See bug #143 ('RFE: Need some cleanup in/for "SUNWastdev" package' - > http://bugs.grommit.com/show_bug.cgi?id=143) for the patch which cleaned > these parts up. > > > cmd/ksh/amd64/Makefile > > cmd/ksh/i386/Makefile > > > > 39-48: use the $(INS.link) rule instead. > > Erm... that won't work AFAIK. $(USRKSH_ALIAS_LIST) is a variable list > and changes based on the switches defined by "Makefile.ksh93switch" (we > support switching the flag in one build to do testing from both sides > without requiring a full rebuild from scratch (remember my fastest build > machine at home needs 8-10 hours to do one build and the slowest chews > almost two days on the tree... ;-( )). I would prefer this solution > since it provides a central place to control the deployment of links and > binaries to avoid having too complex Makefile machinery in too many > places (I've shot myself too often into my feet with this kind of the > Makefiles... ;-( ). > > BTW: Is it possible that INS.link is one of the orphans in the tree ? > http://src.opensolaris.org/source/search?q=INS.link&defs=&refs=&path=%2Fonnv&hist= > only lists two consumers tree-wide... > > > cmd/ksh/Makefile > > > > 58-61: why is i18n dummied out? > > /usr/bin/$(MACH)/ksh93 is just a frontend which directly calls into > libshell.so.1. There is no l10n code... it's all in libshell. > I added a comment in the Makefile. > > > 66-73: looks like $(INS.link). > > Same problem as in usr/src/cmd/ksh/$(MACH)/Makefile: > $(USRKSH_ALIAS_LIST) is a variable list and the tree supports switching > the install mode defined by Makefile.ksh93switch between single builds > for testing (otherwise a rebuild from scratch may be required). > > > cmd/ksh/Makefile.com > > > > 99-101: why is a custom build rule needed? Why not just set up a > > dependency? > > Cloned from the libshell makefile (for the mkdir). See below... > > > 100: yuck. > > The placement of object files follows libshell/libast/etc. where all > object files are stored in subdirs. This is done for consistency in this > case since the "ksh" frontend is in fact a part of the libshell sources > and uses (almost) identical build flags. > > > 105,120: s/expiclitly/explicitly/ > > Fixed. > > > 108: "seems to require"? > > See usr/src/cmd/ksh/Makefile.testshell, the comment about "io.sh" > (originally it was part of usr/cmd/ksh/Makefile.com which would be much > clearer in this case). > > > 126-131: this is what Makefile.lint is about; just remove cmd/ksh > > from the COMMON_SUBDIRS list (it's not there now, by the way), and > > all should be fine. This extra bit of work shouldn't be needed. > > Uhm... last time I tested this the tree didn't work without it. > > > lib/libast/Makefile > > > > 159: I think you might want just "FRC" here, though it's not clear. > > The current build rule will cause make to execute the $(SUBDIRS) > > build rule with an empty $(TARGET) -- building whatever is the > > default target in $(MACH). > > Fixed (in lib/lib(ast|dll|pp|cmd|shell)/Makefile). > > > lib/libast/i386/Makefile > > > > 34: what is this about? What do Solaris distributors need to do > > when updating the minor release number? (And why doesn't the major > > release number fit in here somewhere?) > > That is a #define used by the native AST/ksh93 build to set some > version-specific stuff. The native AST/ksh93 build sets > HOSTTYPE="sol9.i386" for Solaris 9, HOSTTYPE="sol10.i386" for Solaris > 10, HOSTTYPE="sol11.i386" for Solaris 11 etc. and I cloned the behaviour > in the OS/Net Makefiles that we do not have to maintain such values > (AFAIK Mike suggested that we should do that automatically; > unfortunately the OS/Net Makefiles do not provide this value which > forces us to calculate it somehow within our own Makefiles (not perfect > but the value is more or less only used for informational purposes and > won't cause puppy heads to explode when it's wrong)). > > > 47-60: what's the status of the real fix? (I don't really follow > > why this is a special problem for ksh93 and doesn't seem to affect > > anything else in ON -- all of which, varargs and not, goes through > > gcc shadow compilation. Is the bug actually related to the new > > AST-specific varargs?) > > Erm, there are not any AST-specific varargs, the AST code just has to > deal with platform-specific varargs differences. We generate the AST > includes by building AST/ksh93 the "normal" way (via buildksh93.ksh) and > then copy all the generated includes in our tree. The problem is that we > use Sun Workshop/Forte/Studio as compiler - and in this case we run into > a tiny problem since gcc defines it's own flavor of varargs which the > headers do not recognize because they "think" they're dealing with Sun > Workshop/Forte/Studio. We can workaround the problem in two ways: Edit > the generated AST includes or just overriding the defines from "outside" > via the Makefiles. > I strongly perfer the 2nd method because the AST includes are machine > generated (as described above) and each update just "stomps" over the > old includes (plain copy). Local modifications would therefore be very > painfull (e.g. they would need to be documented, maintained and tested > somehow) and a source of constant build/maintaince problems for each > update. > > The real fix is to upgrade to gcc4.0 and/or throw stones after the gcc > people to stop them from using their own varargs header version (both > Sun Workshop/Forte/Studio and gcc varargs are interoperable from the > binary interface, they just use different includes which causes the > hiccups in this case). > > > lib/libast/Makefile.astinclude > > > > 28-29: is this true? > > Yes, that's true... the headers are partially machine-generated based on > "iffe" probes ("iffe means "if feature exists"... like "autoconf" on > steroids) in a native AST/ksh93 build (see above) and then copied over > into the OS/Net tree. > > > (And if it is, how does the comment help > > explain the code that follows?) > > Uhm... what do you mean in this case ? It's a general explanation (BTW: > the comment "i386, sparc" predates the introduction of the 64bit AST > libraries in our tree)... > ... I changed this to: > -- snip -- > # ident "%Z%%M% %I% %E% SMI" > # > > # Note: libast headers are generated by the AST build system outside > OS/Net > # (and then copied here) and depend on the architecture (e.g. "i386", > "amd64", > # "sparc", "sparcv9" etc. ...), we later merge them into one unified > file > # (see below) > > ROOTHDRDIR= $(ROOT)/usr/include/ast > -- snip -- > Better ? > > > 32-38: I don't follow this. What exactly is wrong with _LP64? It's > > set correctly by sys/isa_defs.h -- why would you need to do your own > > version of this logic? > > The problem is <sys/isa_defs.h>. It's a system include in the sys/ > directory. The Sun Workshop/Forte/Studio compiler does not define > |_LP64| which means it's not available unless you include system > includes which interfers with the AST includes ("interfers" is an > understatement... it completely screws-up the AST headers which depend > on precise include order of it's own and the system includes). After > lots of trouble we've settled with avoiding the usage of |_LP64| since > it's not portable enougth (and using -D_LP64=1 at compile time does not > work either (it compiles but the resulting code will be garbage)). > > > 40: What's the justification for having include files that are > > dependent on architecture? This seems too complex by at least half > > to me. > > See above. The includes are partially machine-generated and we just copy > them over However the "iffe"-probes only work for the architecture for > which they are compiled for and the design does not anticipate that one > OS has two different architectures, e.g. 32bit SPARC and 64bit SPARC. To > solve this problem we just compile the native AST/ksh93 one time for > 32bit and one time for 64bit and copy the generated includes over into > the matching directories (even retaining the directory layout for the > generated includes to make maintaince as simple as possible). Finally we > use "diff -D" to create one unified set of include which are then > installed in /usr/include/ast/ (the differences between 32bit and 64bit > Solaris includes are usually not very large but the current approach > gurantees that we don't run into any problems by accident and keeps the > maintaince/upgrade requirements at a minimum level). > > > 56: s/concaternation/concatenation/ > > Fixed. > > > lib/libast/Makefile.com > > > > 33-602: suggest condensing this list with appropriate build rules. > > Erm... what do you mean with "appropriate build rules" in this case ? > Using pattern-matching to collect all *.c files won't work (not all *.c > files are directly compiled or used within the libast code directly). > > > 612,616: these shouldn't be necessary, I think. > > AFAIK they are required because the compiler doesn't create subdirs > itself. Remember my original posting for this wevrev - we put the *.o > files into subdirs to avoid having a few hundred *.o files in one single > directory... IMO the current solution is much more tidy (I wish libc > would use the same method...). > > > 646-650: I don't really follow. Can this be reworded? > > Basically we're worried that changes in $(CPPFLAGS.master) may cause > future silent breakage. Currently $(CPPFLAGS.master) is applied after > the content $(CPPFLAGS) when building the compiler's command line. If > anything in $(CPPFLAGS.master) overrides any "-D" symbols needed by the > AST build we're in serious trouble (we had such a case in the past and > debugging the issue was NOT fun... ;-( ). > Any suggestions for rewording ? > > > 650: s/expliclity/explicitly/ > > Fixed. > > > 653: what does "be careful" mean here? > > Putting the wrong value here can "EOL" older APIs in libast which may > still be needed. This does not affect our libast in OS/Net but the AST > codebase in general, e.g. tools build against libast binaries build from > upstream should work with our libast if possible. > > > 653: s/carefull/careful/ > > Fixed. > > > 682-708: not sure that the comments add much value here. > > Well, they are at least usefull for me that I know where I have to look > at... :-) > > > 709: that's worrisome. > > Uhm... I don't see anything obviously wrong in this code, the code works > and neither "dbx -check access" or "Rational Purity" complain (nor does > gcc) .. therefore I've opt'ed to silence this warning and add it into my > ToDo list for later (somewhere after dealing with this putback, > |posix_spawn()| and "shcomp"&&friends...) ... > > > 715-720: not really needed. > > It seems the build cannot live without that (e.g. "nightly" fails) ... > ;-( > > > lib/libast/Makefile.libastl10n > > > > 36-37: this has to go. We can't support LD_LIBRARY_PATH hacks that > > attempt to run software out of the proto area. > > We know... that's http://bugs.grommit.com/show_bug.cgi?id=118 ("Disable > the usage of tools created during the OS/Net build (ksh93, msgcc > etc.)"). We save the disabling of this code until the last moments > before the official review to make sure we don't break that (we have to > disable it, wait until SUNWastdev is deployed everywhere and then > enabkle this code again without the LD_LIBRARY_PATH hackery). > > I added the following comment section to the file: > -- snip -- > # WARNING: This has to go (see bug #118 # ("Disable the usage of tools > # created during the OS/Net build (ksh93, msgcc etc.)" - > # http://bugs.grommit.com/show_bug.cgi?id=118)). > # We can't support LD_LIBRARY_PATH hacks that attempt to run software > # out of the proto area. > -- snip -- > Better ? > > > lib/libast/mapfile-vers > > > > 29-618: unhelpful comments; find another place to document the > > history. > > Where should I put this information ? The *.diff file, touristguide.xml > etc. are currently under the "we don't want/have/tolerate such files > (kill them!! burn them!!) in the tree"-attack, remeber ? AFAIK we have > to settle that discussion to find a (new ?!) place where the > putback-specific documentation can be stored... ;-(( > > > lib/libcmd/i386/Makefile > > > > 35: please don't add stray blank lines at the end of the file. > > Fixed for lib/libcmd/(i386|amd64|sparc|sparcv9)/Makefile > > > lib/libcmd/sparcv9/Makefile > > > > 28: not sure why this is needed. > > All Makefiles used for libast/libpp/libdll/libcmd/libshell/etc. use > SHELL=/bin/ksh > > > lib/libc/port/regex/wordexp.c > [snip] > I'll move the workexp.c parts into a seperate email thread... I guess > may become a little bit longer... ;-( > > > lib/libshell/misc/buildksh93.readme > > > > 33: s/it's/its/ > > Fixed. > > > 71: missing "##"? > > Fixed. > > > > > pkgdefs/SUNWarc/prototype_com > > > > 75-76,113-114,182-183: something seems out of sync here. If you're > > not shipping the compilation symlinks (the lib*.so symlinks), then > > why ship the lint libraries? How is linting source that cannot be > > linked useful? > > (See SUNWcsl; it has only the *.so.1 files, not *.so links.) > > Known issue. Solaris has the strict rule that we're not allowed to ship > any *.so files if the API is not public, therefore we do not ship the > *.so files. However some people may be interested to play around with > the API (e.g. to build ksh93 plugins (builtin commands, functions etc.)) > ... the *.so links can easily be restored by hand but the lint files > cannot be "restored" if we don't ship it. That's why we're ending up > with a weired chimera shipped with three legs where leg No. 4 can be > substituted with a wooden one on demand (erm... Ok... this metapher > isn't a good one... ;-( ). > > > pkgdefs/SUNWarc/prototype_i386 > > pkgdefs/SUNWarc/prototype_sparc > > > > As above; can't tell why lint libraries are shipped. > > See above, the three-legged chimera. Basically we honor the rules and > still try to keep the value alive somehow... :-) > > > pkgdefs/SUNWastdev/depend > > > > Curious: how was this list determined? > > Oh-oh... OH-OH... *duck*... > ... seems this was done by turning off the brain, copying the package > files from one of the perl packages in pkgdefs/ and then thinking I can > get away with this thing. Fixed with > http://bugs.grommit.com/show_bug.cgi?id=143 (yes, this was really > dumb...) > > > pkgdefs/SUNWastdev/Makefile > > > > 32: you have your own 'depend' file. There shouldn't be a > > dependency on it here! Please remove, as it'll cause source-only > > builds to fail. > > See above. Fixed with http://bugs.grommit.com/show_bug.cgi?id=143 > > > pkgdefs/SUNWcsu/prototype_i386 > > > > 106-107: alphabetic order, please. > > Fixed. > > > pkgdefs/SUNWcsu/prototype_sparc > > > > 55-56: alphabetic order. > > Fixed. > > > pkgdefs/SUNWhea/prototype_com > > > > 165-166: I think 's' comes before 't', so 'assert.h' comes before > > 'ast'. > > Fixed. > > > pkgdefs/SUNWosdem/prototype_com > > > > 60-99: alphabetic order, please. > > Fixed. > > > Targetdirs > > > > With the exception of line 223, all of the new entries in this file > > look wrong to me. The "REALPATH" goop is needed for libraries that > > are in /lib (in the root file system). That's not true of *ANY* of > > the new AT&T libraries. They're all over in /usr/lib. Thus, no > > magic "../../lib/" symlink (referring to the root file system) is > > needed. > > Umpf... that's an artifact of the part which got ripp'ed out during > PSARC 2006/550 where the original plan was to target delivery into /lib. > Somehow this was sacrified (at the expense of having another ARC case > just to move that back again to /lib "on demand") and all the ksh93 libs > suddently found themselves in /usr/lib instead... ;-(((( > > Fixed. > > > How does the software even build with these rules present? (It > > should fail ... but I guess it's possible that there are bugs that > > prevent it from failing ...) > > AFAIK it only creates the links and nothing in the tree (including > "makebfu") tests whether links are correct or not (and the build rules > use $(RM) to remove targets before writing new files (and that removes > these links)) ...
James: ping! Are the comments/changes "Ok" for you (excluding *.diff and "wordexp.c") ? ---- Bye, Roland P.S.: New webrev can be found at http://mail.opensolaris.org/pipermail/ksh93-integration-discuss/2007-February/002164.html ... -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;)
