Roland Mainz writes: > 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/ [...] > > > 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.
Yes, and ok. > [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. [...] > > 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...) ? That's how we handle other things, such as isaexec. Controlling build order and dependencies is pretty much the reason 'make' is there. > [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... :-) )). That doesn't seem the most convincing of arguments to me. I guess I'll need to cease caring too much. > [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). I don't see how that's really an issue at all. First of all, you'll need to tweak the packaging when you get to that day. The current packaging won't support what you're doing after you flip the switch -- so more is needed anyway, and the switch code isn't logically complete. Secondly, if you're regenerating those links on demand, then in what possible case are the dependencies wrong? (It seems like that's what you're assuming here -- that the dependencies end up being wrong and the wrong thing gets linked -- but I just don't see how that's possible.) > > > > cmd/ksh/Makefile.com [...] > > 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)... I don't see how causing hundreds of "mkdir -p" invocations in a custom-crafted build rule is any less complex. I suspect that the implication is that you feel that makefile syntax itself is just too hard to maintain, and that you'd rather just have a script. Is that the issue? > > > > 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)). I still don't understand. Either it does require it, or it does not require it. Which is true? "Seems to" implies that the design work isn't done yet, and that you don't really know how (or if!) the test code works. Please clarify. > > 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: I understand that it maintains it "automatically." > ... 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) I see. But what would they do with it? I still find this logic to be baffling. What's the usage case? Why does this matter here? > 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)) That's just frightening. Did I miss that in ARC review? > > 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). And, yet, that simple renaming confuses gcc ... ? That seems like something worthy of a little investigation. > > > 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|. ... which is why I said "every place that matters." Oh, well. > > 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. I was suggesting doing it whereever it makes the most sense. If that's upstream, great. > > - 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...) ... OK; as long as it can go then, I guess I can drop it. > > > 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 ? Because you're saying that you can be blown out of the water by ordinary -- even standards-conforming header files. That doesn't bode well for the future. > > 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 -- Right; which is why you'll find that in sys/isa_defs.h on Solaris. It makes certain that _LP64 is *always* available. > ... 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)). I must be missing something. I still don't understand why including sys/isa_defs.h is a problem. Is that file toxic in some way? If so, isn't that a bug (either in Solaris or in AST or both)? If it's not toxic, then why would it be the case that it can't be included, at least when you're on Solaris? I just don't understand why you need to reinvent this mechanism. This is the sort of hack that'll come back to haunt us later when ports to other platforms end up being much harder than expected. > > > 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 Sorry, but that doesn't answer the question. It contains no technical details describing what the problem might be. Theory is nice, but understanding the problem is better. > > > > 40: What's the justification for having include files that are > > > > dependent on architecture? This seems too complex by at least half > > > > to me. [...] > > 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). OK. > > 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). As I've said many times, the "just integrate" plan is exactly what SFW is all about. ON has its own rules. > > > > 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)). It certainly doesn't look difficult to me -- they're separate makefiles. > 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)) ... Someone's going to be stuck maintaining that huge list. > > > > 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)). Ugh. > > > 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. So what? It's the object directory -- not the source. Why does it matter whether the object directory is "tidy?" > > > > 646-650: I don't really follow. Can this be reworded? [...] > > 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. So ... it's just arbitrary. > > 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). I see. > > 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) ..." ? That'd do. > > > > 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. Who will actually keep those comments in sync with the code if someone adds or deletes a line? In my experience, stuff like this just rots over time. It's obsolete by the time it gets checked in. It's just plain noise. > > > > 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. The line numbers don't match up -- line 731 is blank. I don't see how the code you're citing could cause the error you're seeing, because what you've declared (for the 'p' variable) is a pointer to storage that is const -- and not a const that itself needs an initializer. I think that's worth a bug on the compiler, if it's real. Something's wrong here. > > > > 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. In general, we've written makefiles in ON that assume that 'make' itself works, and that the shell is extremely minimal (Bourne). It worries me a bit that someone is wandering outside if this area deliberately ... but I can't give a specific case where I think it'll necessarily fail. > [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... ;-( Thanks. Though bleach would do. :-/ -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 1 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
