Roland Mainz writes: > James Carlson wrote: > > April Chin writes: > > > In particular, we're looking for code reviewers who can take a look at > > > the non-Makefile files in > > > http://cr.grommit.com/~chin/ksh93-webrev-jul9/jul9-nonattfiles [...] > > - The usual way for handling non-working lint targets is to make [...] > > The current scheme works, but could be better. > > The current scheme tries to avoid that people run into trouble when they > manually walk through the directories and do a "make lint" (see "perl" > build for a precedent) and then start to cleanup the lint warnings (see > earlier comments about a "lint [warning cleanup] party" ...
OK. There are issues here other than lint, though. Someone could decide to do a "printf cleanup party" or a "spelling fix party" and run into exactly the same problem. The generic issue is that we have some sources where changes need to be fed upstream -- not just lint. > > - It'd be nice (though not required) to have helper scripts to do > > the grunt work required during the build process. In particular, > > $SRC/cmd/kes/Makefile.testshell is pretty hard to read, > > Why ? It's only one extra layer of esacping rules (for example > Imakefiles have at least two layers) ... :-) It's just the "ick" factor. It's hard to read and thus hard to maintain properly. We could also transliterate it into Korean, but that probably wouldn't help much, either. :-> > > with a > > bunch of escapes that are complicated by the fact that it's coded > > in the middle of a makefile. Two or three lines are ok, but 58 > > lines seems like a lot. > > Erm... the problem is that it needs _lots_ of variables from the main > Makefile Yes, I see that. It needs "several." > and an external script would still require access to all those > makefiles, e.g. you won't save much by replacing the current > "in-Makefile" script with a giant list of variables passed to a script. > And the script has 58 lines but around ~~18 extra lines are needed to > squish the script into the 80-column format... ;-( > ... but: If we switch SHELL=/usr/bin/ksh over to SHELL=/usr/bin/ksh93 at > least ~~8-12 lines can be removed (e.g. all the "egrep" usage, the > weired "while read ; print $REPLY ..."-loop etc.) Still, when possible, I prefer to see largish scripts delivered separately from the makefile. > Anyway... for now I would prefer the current implementation (= without a > seperate script) because it is IMO easier (at least for me (and we > already put the rules into a seperate Makefile include to avoid that > normal engieers die from looking at it by accident... =:-) )) ... OK. > > - $SRC/cmd/sgs/libelf/Makefile.targ: why is a .WAIT needed before > > $(ROOTDEMODIRS)? It looks like it should go with the rest of the > > directory targets. (Probably just a nit.) > > Erm... the ".WAIT" rule should make sure that the directories really > exist before creating files into it, e.g. first create dirs, then > populate it. I don't follow. $(ROOTDEMODIRS) is itself delivering a directory, and nothing else. Does that directory depend on any of the directories that appear before it in the list? I don't think that it does. I'm asking about the .WAIT that is *before* the $(ROOTDEMODIRS), and not the one after. > > - The private libc buried in libast (unlinted, no less) > > We're working on the lint part right now (and I still own David an email > with feedback and I have to file a bug against Sun Studio 11's "lint" > for reporting bogus warnings in C99 mode... ;-( ) ... OK. > > is still > > frightening. Not an issue to hold up integration, but I hope that > > something better will be done in the future. Supporting more than > > one libc in ON seems like a hazard. > > Well, there are at least four items why this part is there: Yes; I'm well aware. But in giving final comments, I still feel I'd be remiss if I didn't mention that I think this kind of duplication is a mistake, no matter what the intent. > - Solaris libc functions not conforming to POSIX/XPG standards (this is If you find any standards conformance problems, please do file bugs. > - Historical limitations of the libc implementation (for example the Also, file bugs. > Solaris libc stdio implementation where is no way to integrate the AST > stdio functionality without breaking backwards compatibilty (which is a > _pain_ because the AST stdio implementation is much more flexible and > _FASTER_). In theory there is the option to provide a build-time switch > and build all new applications with an all-new stdio implementation... > but I don't even dare to imagine how the matching ARC case would look > like (war ? bloodbath ?) ...) Having two of them seems like the worse answer. I'd like to have one. If moving some or all libast things into libc is the answer that solves the duplication, then it sounds like a possible future way forward to me. I don't see why you're assuming there'd be a "war" here. I don't get it. > - libast's functions have lots of extensions, for example the regular > expression functions support more pattern matching modes (e.g. > perl+shell expressions are currently supported and more may be added > later). In theory this could be moved over to libc (and likely very > welcomed by the community) but ARC'ing will be tricky because of some > problematic restrictions in the libc code (which could be avoided by > leaving the old code alone and only enable the extensions in >= XPG6 > mode... but I am not sure whether the standards people at Sun would be Ick. It'd be better to have extensions for all. I understand if it's just impossible, but I'm less understanding if it's merely "problematic." In any event, this sounds like detail for a future project, not something to resolve here. > happy to have "extensions" in the "XPG6 standard mode") We've done it in other areas. XPG _allows_ extensions. > - Someone has to maintain the code. libast is maintained by David, Glenn > and the AST community, including further development. If we put the > functions into Solaris libc - who would do the syncing with upstream > and/or doing further enhancements ? Actually, you're putting libast into ON. Someone has to pull changes from the AST community and get them into ON. That doesn't happen "automatically," and it's not done by the AST people. That same someone who updates ON can very likely deal with things if the same files are in different locations. I don't see that as a substantial issue. > > - No changes were made to these files: > > $SRC/lib/libcmd/amd64/Makefile > > $SRC/lib/libcmd/i386/Makefile > > $SRC/lib/libcmd/sparc/Makefile > > $SRC/lib/libcmd/sparcv9/Makefile > > Don't deliver unchanged files. > > What do you mean ? Take a look at the diffs -- there are no changes to those files. They need to be removed from the workspace and reverted to the original in the gate. > > - $SRC/pkgdefs/Makefile: looks like a missed merge. (Not a code > > review issue, but you'll want to clean it up before going for > > RTI.) > > Erm... what do you mean ? Just a construction error in building the webrev. April needs to make a new one. -- 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
