James Carlson wrote: > Roland Mainz writes: > > > > http://www.nrubsig.org/people/gisburn/work/solaris/ksh93_integration/ksh93_integration_prototype004_webrev_20070127/non_ast_files/webrev/ > [...] > > > 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: > [...] > > > > 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) ... > > OK; but that means that separate build rules aren't needed now, right?
Do you mean: -- snip -- -.c.o: - $(COMPILE.c) $(OUTPUT_OPTION) $< $(CTFCONVERT_HOOK) - $(POST_PROCESS_O) -%: %.c - $(LINK.c) -o $@ $< $(LDLIBS) - $(POST_PROCESS) - -- snip -- ? If "yes" ... http://bugs.grommit.com/show_bug.cgi?id=143 removed this piece. The only thing which remains are the explicit build rules for the scripts which differ in the content they add to the scripts. [snip] > > > 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 > > Part of the (seemingly unnecessary) complexity here is that > $(USRKSH_ALIAS_LIST) includes $(PROG), and this little script exists > in part just to skip over that one entry. > > Perhaps it's just my preference, but I'd rather see traditional > makefile constructs in makefiles, rather than resorting to shell > scripts to do common things. And forcing $(SHELL) to something other > than /bin/sh seems doubly strange. > > In this case, I probably would have done this in Makefile.com (note > that I've removed $(PROG)): > > $(BINKSH_IS_KSH93)PROG= ksh > $(BINKSH_IS_KSH93)USRKSH_ALIAS_LIST=ksh93 rksh rksh93 pfksh > > $(BINKSH_ISNOT_KSH93)PROG= ksh93 > $(BINKSH_ISNOT_KSH93)USRKSH_ALIAS_LIST=rksh93 > > And then something like this in the Makefile: > > INSLINKTARGET= $(ROOTPROG64) > ROOTLINKS64= $(USRKSH_ALIAS_LIST:%=$(ROOTBIN64)/%) > > install: $(ROOTLINKS64) > $(ROOTLINKS64): $(ROOTPROG64) > $(ROOTPROG64): all Erm, correct me if I am wrong... this would wallpaper hardlinks unconditionally over the frontend binary which makes this depend on the build order. If the wrong order is used you have lots of links but no frontend binary, right (OkOk... we could enforce this via adding some ".WAIT" statements... but see below...) ? [snip] > > 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... > > I agree that it's not well-used. If you don't want to use it, I > suppose that's fine. > > My reaction was mostly to the blob of shell code appearing in the > midst of a makefile. That's sometimes necessary, of course, but I'd > rather see it minimized if at all possible. I agree... but sometimes it's clearer to use the script code to avoid that it's getting to complex. You missed the stuff we had in the tree which once allowed placing ksh93 into /sbin/sh's place (a seperate, 32bit-only copy of the frontend). First we tried it with Makefile-only rules which was completely mind-melting and only understandable after drugging myself up with large amounts of coffee. Finally I've ripped the whole thing out and replaced it with the small mini-scripts because this was something easier to maintain (and less prone to race-conditions and other "fun" stuff... (and more important... I could understand this even at 5:00AM without lots of coffee... :-) )). [snip] > > 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). > > As above, I think the complexity here is in having $(PROG) appear > inside $(USRKSH_ALIAS_LIST), not the way in which the switch is made. The problem is that $(PROG) changes and that we don't know whether the frontend will be stored at /usr/bin/ksh93 (making /usr/bin/ksh a hardlink) or /usr/bin/ksh (making /usr/bin/ksh93 a hardlink) in the final post-migration world (Ok, it's a long way down the river of tears util we reach that.... but it's still usefull for testing). > > > 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. > > That's not what I was "yuck"-ing. My comment was on the mkdir -p > buried in here; it's very unmakefilish. (makefilian?) > > I'd prefer something roughly like: > > COBJS= \ > sh/pmain.o > > OBJDIRS= sh > > $(OBJDIRS): > $(MKDIR) $@ > > $(PROG): $(OBJDIRS) .WAIT $(COBJS) Ok, but see the comment about libast's Makefile.com below. The basic idea is to keep the complexity down and avoid per-directory rules (it may sound like an overkill for usr/src/cmd/ksh/Makefile.com but remeber this file was carved out of usr/src/lib/libshell/Makefile.com and should follow the same logic)... > > > 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). > > I'm unclear on the use of the phrase "seems to." > > Does it indeed _require_ that ksh93 is on the path as "ksh," or does > it not require that? It seems to require that the test suite is always called with SHELL=/path/to/binary/ksh, therefore we hardlink /path/to/binary/ksh93 to /path/to/binary/ksh (or reverse for build symetry (to avoid that Makefile.ksh93switch bites us if we switch in one build)). [snip] > > 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)). > > I don't understand why this is needed at all. If it is needed, I > don't see how it can live with just the _minor_ version number, and > completely ignoring the major version number. > > Moreover, we're only building for one version of Solaris here. It's > not possible for the code in the S11 tree to build S9 or S10 bits. Right. But the current solution avoids that we have to maintain the value at all - it's calculated automatically (maybe the tree should keep the value in usr/src/Makefile.master near the following definitions: -- snip -- ONVERS= "11.11" RELEASE= 5.11 -- snip -- ... if gatekeepers agree I can add the value here instead, e.g. -- snip -- ONVERS= "11.11" RELEASEMINOR=11 RELEASEMAJOR=5 RELEASE= $(RELEASEMAJOR).$(RELEASEMINOR) -- snip -- ... but I would prefer to do that as part of the "OS/Net Janitors" project because this is a tree-wide change and we avoided such things for this putback to prevent any possible risk, even keeping the ASTPROG&co. rules seperate from Makefile.master for now. ). > I'm not asking whether the upstream does something like this; I'm > asking _why_ it's done, and what purpose it serves. $ ggrep -r HOSTTYPE $PWD # shows the following locations -- snip -- usr/src/lib/libast/common/port/astconf.c: "HOSTTYPE", usr/src/lib/libast/common/port/astconf.c: HOSTTYPE, usr/src/lib/libast/common/path/pathprobe.c: p = path + sfsprintf(path, PATH_MAX - 1, "%s/.%s/%s/", p, probe, HOSTTYPE); -- snip -- 1. astconf.c is used for the |libast::astconf()| call to return this value as platform identifier (that applications and/or scripts can use this) 2. usr/src/lib/libast/common/path/pathprobe.c uses this value to lookup user-specific code in ${HOME} using the value from HOSTTYPE as platform-identifer (since ${HOME} may be shared across multiple architectures (this isn't used directly in libshell/ksh93 but may affect future libast-based applications)) > > > 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, > > What is `_ast_va_list', which is cited in the bug report? That looks > to me like an AST-specific implementation of varargs. Is it actually > something else? >From /usr/include/ast/ast_common.h -- snip -- #define _ast_va_list va_list -- snip -- ... it's a portability #define (see below). > > the AST code just has to > > deal with platform-specific varargs differences. > > Why would it need to do that? Doesn't <stdarg.h> do the right thing > in every place that matters? <stdarg.h> only works for platforms which conform to the matching ANSI/ISO C standard. The AST software runs on more than 40 platforms and not all of them heared about something like "standards" and/or implement them correctly. The code needs to be portable and therefore uses |_ast_va_list| since some platforms do not have a ((propperly) working) |va_list|. > > 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. > > There seem to be at least two other possible fixes here: > > - Revert to using standards-based <stdarg.h> and abandon the > independent and likely unnecessary version. Erm... that would completely ruin portabilty of the code and we would more or less fork() the whole tree. The time required to do that is completely beyond my powers (and we try to avoid fork()'ing the code, rememeber ?) and would ruin our abilty to keep our sources in sync with the upstream sources. This is why I am avoiding touching the autogenerated includes and treat them as upstream sources, too. We can't really afford fork()'ing this part without eliminating our abilty to keep the sources in sync with the upstream sources. Such a fork() would quickly lead to the death of this project because it would require much more time to keep things in sync. > - Get a fix for that lousy gcc "mkinclude" design flaw. It's awful, > and breaks things other than AST (including OS patches). AFAIK there was an annoucement to port the OS/Net tree to gcc4.x a while ago... but I guess this will need some time and I really hope our putback comes first (I am praying...) ... [snip] > > > 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). > > That sounds like a potentially serious design problem to me. Why ? > You also get those <sys/*.h> things just by using "ordinary" include > files, such as <stddef.h>. Yes, but those are used later in the include chain. <sys/isa_defs.h> is a platform-specific include and therefore not portable. gcc makes this much easier since it #define's |_LP64| for 64bit platforms, e.g. -- snip -- $ (touch foo.h; /usr/sfw/bin/cpp -m64 -dM foo.h) | fgrep _LP64 #define __LP64__ 1 #define _LP64 1-- -- snip -- ... unfortunately Sun Workshop/Forte/Studio do not do that and that limits our choices. Defining |_LP64| in our own Makefiles doesn't work since we would have to modify all consumers which uses the AST includes, too (and that includes future external software, too (which expect that our includes work identical to the upstream 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)). > > I'm very leery of this. Not because I think _LP64 is the greatest > thing since sliced bread, but because I suspect it points to deeper > design flaws that will end up producing unstable builds. Uhm... we had never problems with that (and as I said this is working across more than 40 platforms and I really won't call such a thing a "design flaw" (if you want to throw bricks aim at all the platforms which think that standards are an optional feature... ;-( )). > How is it possible that AST is so dependent on system internals that > use if <sys/isa_defs.h> "screws-up" the compilation? See Glenn's explanation at http://mail.opensolaris.org/pipermail/ksh93-integration-discuss/2007-February/002191.html > > > 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). > > Then this is something that should be fed upstream: a split like that > is ugly, likely unnecessary, and will undoubtedly cause trouble in the > future. Yes, it's on the ToDo list but right now we can't fix it ad-hoc, AST is a large codebase and can't change it's APIs over night (estimated amount of time to get such things "fixed" is around 3-4 years/enginneer). > That's the sort of thing I'd expect to find in SFW, not ON. What should we do instead ? The current approach has at least been proven to work for the last year across many many updates and a redesign of the whole AST codebase is a little bit more work than most people imagine (this one of the reasons why this API is not "public" yet). > > > 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). > > No, I don't mean collecting all the *.c files. > > Instead, I might have build rules that look like: > > objs/%.o: common/cdt/%.c > > and an object list that says: > > CDTOBJS= dtclose.o dtdisc.o dtextract.o ... > > COBJS= $(CDTOBJS) $(COMPOBJS) $(DIROBJS) ... Uhhrghh... this would be quite messy (look at libast//$(MACH)|$(MACH64))/Makefile, you have to handle the platform-specific sources, too (and avoid that this crossfires)). I would prefer something which does this stuff automatically (BTW: The list of object files is fetched from the output log of "buildksh93.ksh", filtered via various things, passed through /usr/bin/sort and then put into the Makefile... this procedure keeps maintaince low, too (otherwise you have the deal with the usual "where is my missing symbol"-search for some larger updates)) ... > > > 612,616: these shouldn't be necessary, I think. > > > > AFAIK they are required because the compiler doesn't create subdirs > > itself. > > No, but your makefile should by using _targets_, not by invoking > "mkdir -p" several hundred times during the build. The only thing which would work is a rule which dismantles $(COBJS) and creates the subdirs as dependicies. The problem is that this is MUCH slower than the single "mkdir -p" (on my machine processing 600 * 2 * 2 * 2 rules/targets is slower than 600 * "mkdir -p" (mkdir is even a builtin in ksh93 making the thing even faster)). > > 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...). > > It certainly adds complexity, and I don't see how it's really helpful. At least I consider this setup "cleaner" since you don't have 600 or more object files in one flat directory. Instead it's all nicely stored in subdirs. > > > 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 ? > > Why would $(CPPFLAGS.master) be overriding anything you need for the > AST build? > > I don't quite understand why this is an issue for AST in particular, > but not an issue for most other things in the ON gate. What's special > about AST here? > > The only things I see in $(CPPFLAGS.master) (of any importance) are > -D_TS_ERRNO and -DTEXT_DOMAIN. Do those harm AST? No, they don't harm. But future content may harm and that's where we're afraid about (I already visited this special hell and do not want to see the boiling pits of lava again...) - see below. > Do you need to assume that folks updating the ON software don't verify > and test their work? Or is it more complex than that? Finding minor hiccups in the tree may be very difficult, not even the AST/ksh93 test suite can catch all the tiny problems. Long ago I've got this wrong and then spend almost a week to track the problem down. I really do not want to see this xx@@@@!!! again. Really. (And as usual: The test suite is not part of the default build anymore and I really doubt that the pre-integration tests will catch special problems in this area in the first run. Once something bad happens here it will take weeks or months getting the problem found&&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. > > I'd suggest rewording to explain what it is maintainers need to be > careful about. Something like: "do not change the __OBSOLETE__ value > without examining the symbols that will be removed, and evaluating > whether that breaks compatibility." ... maybe adding as 2nd comment: "... make sure this is in sync with upstream (see Mamfile) ..." ? > "Be careful" doesn't say enough. I'd expect _ALL_ people coding in ON > to be careful ... all the time. ;-} ;-/ > > > 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... :-) > > It takes about three seconds to check out the file and remove those > overrides. ... and around an half a hour to get usr/src/lib/libast/ compiled (in my case) ... ;-( ... I would prefer to keep the comments that I don't have to recompile all the stuff just to get the warnings back. > > > 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...) ... > > A const without initializer doesn't make any sense to me. What does > the code look like? (I don't seem to have access to it.) See http://www.nrubsig.org/people/gisburn/work/solaris/ksh93_integration/ksh93_integration_prototype004_webrev_20070127/allfiles/webrev/ for a webrev covering all files (the webrev's are split into categories). The code looks like this: -- snip -- 707 register int c; 708 char* e; 709 const Prefix_t* p; 710 -- snip -- s/const Prefix_t\* p/const Prefix_t\* p=NULL/ fixes the problem. [snip] > > > 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... ;-(( > > "README" is fine, if you feel the need. > > In this particular case, this all looks like ancient history to me. I > don't see what lasting value it has. This is not really ancient history, I didn't had time to rewrite the generation script and test it (remeber the AST codebase is _huge_ and testing all the applications needs some time). > > > 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 > > But why do they care? Erm... what do you mean ? SHELL=/usr/bin/ksh is used uniformly across the new Makefiles added to make sure we can use the normal POSIX-shell syntax, including nested brackets (e.g. $ ( foo ; ( bar ; zapbambam ; ) ; meepmeep ) #). I would prefer /usr/xpg4/bin/sh but I doubt gatekeepers would be happy to have a dependicy on /usr/xpg4/bin/ stuff without a larger pro/contra discussion. That's why I picked /usr/bin/ksh instead (IMO it would be nice to put something like that in Makefile.master but that's more the domain of the "OS/Net Janitors" project to cleanup things) and aim to replace that with SHELL=/sr/bin/ksh93 together with the step which enables the building of the AST l10n files. [snip] > > > 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... :-) > > I don't think they ought to be shipped. Ok... ;-( Lint libraries are now scheduled for execution in boilling acid... ;-( ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) [EMAIL PROTECTED] \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;) _______________________________________________ opensolaris-code mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
