Roland Mainz wrote: > > James Carlson wrote: > > April Chin writes: > > > The webrev for all of the other (non-AST) files, including Makefile > > > and packaging changes, is at: > > > > > > http://cr.opensolaris.org/~chin/ksh93-update1.non-ast > > > > By request, I looked at only makefiles usr/src/cmd and usr/src/lib. > > (In a few cases, I veered off that path to sample some of the llib and > > mapfile changes.) > > > > Some general issues: > > > > It can be helpful, especially for huge sets of changes like this, to > > use the "-w" option in webrev to assign particular comments for > > individual files. Obviously, Mercurial doesn't like to track things > > that way in the actual commit logs, but doing this with webrev can > > help out reviewers who are trying to match up CRs and ARC cases > > against actual code changes. > > > > If there are no substantive changes in a file, then the file > > shouldn't be part of the review nor part of the eventual > > integration. In particular, there's no reason to rototill the CDDL > > and copyright text alone in any file. > > > > The libraries look like they need some additional TLC. I'm guessing > > that *might* be something on a future development path, but if not, > > then I'll flag some of those issues here. > > What is "TLC" ? > > > usr/src/Makefile.ast > > > > 38-39: I'm not certain I know what these new directives are for, but > > it might be nice to add "install_h" targets for these > > directories so that the header files get installed in the > > proto area, and thus you won't need reach-around "-I" > > directives. You don't have to place everything that's in the > > proto area into a package for delivery; that's what the > > exception lists are for. > > The issue is that the headers are private to libshell (the "ksh" and > "shcomp" frontends are part of the libshell source but we compile this > stuff in usr/src/cmd/) and are platform and architecture-specific and we > use the upstream ordering to source the includes. > > > (As a matter of general practice in ON, that's what we do: we > > put our private header files into $ROOT/usr/include along > > with all the rest, and then use the packaging definitions to > > determine which things actually get delivered and which are > > just used as part of the build alone.) > > Ok... but in this case even the upstream build system (which has a > concept of private, semi-private and public headers) keeps this stuff as > "private" since it is platform+architecture-specific. > > > (If the header files in those special directories somehow > > conflict with or corrupt something else, and thus have to be > > hidden away out of $ROOT, then ignore this comment.) > > > > 67: I'm not sure I know the importance of this change. April (or > > someone else associated with the project) should look into the > > legal issues to make sure that this name change doesn't cause > > trouble. > > April already asked the lawyers long ago, AFAIK it was "ok" ... > ... April ? > > > usr/src/Makefile.ksh93switch > > > > There are no substantive changes here. Is this part of the review? > > Technically we only updated the CDDL stuff since the file is part of the > project. > > > usr/src/cmd/Makefile > > > > 388,699: it looks like you've deleted "sum" from this list, but have > > not deleted the contents of the usr/src/cmd/sum/ directory. > > Fixed. > > [snip] > > usr/src/cmd/ast/msgcc/Makefile > > > > 48: is this really needed? (Do you have Japanese source code?) If > > it is needed, then a comment about the extra options used would > > be nice to have; perhaps right before line 46. > > The issue was AFAIK discussed earlier: OS/Net switched to Sun Studio 12 > recently and the compiler sometimes (~~one out of 300 compiler runs) > chokes and dies with an internal error (yes, this is under investigation > and we'll try to escalate the issue). The "-xcsi" option prevents it. > And "no", we don't have japanese C sources... but in the future it may > be nice to consider a switch to en_US.UTF-8 as default encoding in > OS/Net to allow non-ASCII math symbols in the sources for documentation > purposes. > > > 54: out of curiosity, what stops it from using shcomp now? (I > > assume it's that you can't assume the build machine has it, > > because this project is actually what installs shcomp, and you > > don't want to build a native copy just for the build process. > > Right?) > > The issue is that "shcomp" needs to be available on the host machine > (the libshell we ship since B72 would be sufficient _except_ that > mapfile-vers blocked one mandatory symbol... ;-( ) and the host system > needs to be able to execute the bytecode, e.g. the host system needs the > "shbinexec" kernel module for that. > > > usr/src/cmd/ksh/Makefile > > > > 32:nit: 'pfrksh' seems like an odd combination of beasts to me. > > See http://bugs.opensolaris.org/view_bug.do?bug_id=6763029 ("restricted > profile shell option (pfrsh) would be convenient for setting up > restricted accounts"). The idea is to have "profile shell" and > "restriced shell" mode active at the same time. ksh93 now properly > detects this condition based on the executable name being used. > > > 67-77: I still wish these were done with normal make targets like > > everything else in ON where we build symlinks, rather than as > > in-line shell scripts. I know we've been over this ground > > before, but using extensive shell scripting inside makefiles > > is just plain ugly to me, particularly when make works well > > without it. And, no, I don't care a whit about "performance" > > for a trivial loop that only runs two or at most 6 times per > > build. > > The issue in this case is to get it working properly with the switch > stuff in Makefile.ksh93switch and the exception for "PROG". I'm not sure > it's even possible to do it with plain Makefile constructs. > > > usr/src/cmd/ksh/Makefile.com > > > > 31-35: why do these lists need to be copied in both the top level > > makefile and here? It'd be nice to have them in exactly one > > place. Perhaps the top-level makefile > > (usr/src/cmd/ksh/Makefile) should itself just include this > > Makefile.com. > > This doesn't work. In theory we could add another Makefile include file > for this information but IMO this is an overkill - the lists don't > change that often, the people working on the code know about both > locations and everyone not knowing it will receive a bite by "makebfu" > when the files are missing... :-) > > > > 61,65: more character set mystery; need at least a comment. > > See above... > > > usr/src/cmd/ksh/Makefile.testshell > > > > 95-163: for anything over a line or two, I strongly suggest moving > > it into a separate shell script that then is invoked by the > > makefile as needed. This style (with embedded scirpts) is > > far less readable, particularly because of the quoting > > requirements of makefiles. Too much of this looks like line > > noise. > > Same answer as during the original ksh93-integration putback: The script > code relies on lots of Makefile variables (AFAIK 18 if I count this > correctly) which we would have to pass as arguments somehow. In that > case the size of the script would double the line-count (for argument > parsing since we can't pass these variables via the environment without > running into side-effects) and the Makefile.testshell wouldn't much > shrink in size and the whole thing would be even more complex. > > The plan was to rewrite the Makefile.testshell to use compound variables > and record-oriented pipes which would greatly reduce size and > complexity... the problem we hit was that we found some bugs (which are > now fixed and guarded by a couple of new ksh93 test suite modules (e.g. > sun_solaris_vartree001.sh, sun_solaris_vartree002.sh, > sun_solaris_vartree003.sh, sun_solaris_local_compound_nameref001.sh and > sun_solaris_compoundvario.sh)) - which means we can do the re-write only > _after_ this putback is done. > > > usr/src/cmd/ksh/amd64/Makefile > > > > 35-44: apparently duplicated logic from the top-level makefile; > > something is odd here. (I'd expect either every > > per-architecture makefile to install the symlinks *OR* the > > top-level makefile to do it, but why should both be attacking > > the problem?) > > The logic is different between 32bit and 64bit. I moved this into a > per-$(MACH) rules ("INSTALL.ksh.32bit" and "INSTALL.ksh.64bit") which > now live in Makefile.com > Fixed. > > > usr/src/cmd/ksh/builtins/Makefile > > > > 64-67: there's an existing $(INS.link) that does this; please use > > it. > > Fixed. > > > usr/src/cmd/ksh/i386/Makefile > > usr/src/cmd/ksh/sparc/Makefile > > usr/src/cmd/ksh/sparcv9/Makefile > > > > 35-43: as above, unexpectedly duplicated logic. > > See above. > Fixed. > > > usr/src/cmd/shcomp/Makefile > > > > 82-87: if you're not using *.po files, then why is this needed? > > Things not using them are usually just skipped over for the > > msgs build -- and if it is needed, then whatever is > > descending in here and trying to build or use *.po files > > should be stopped. > > The last time I had my nose in this logic I realised that it would need > a large-scale change of half the tree for that (e.g. add a per-consumer > Makefile variable to define whether *.po processing is intended or not) > or change the matching target to drag some script code with it. The > original suggestion by gatekeepers was simply to add the dummy target. > > [snip] > > usr/src/lib/Makefile.asthdr. > > > > #include <std/comment-regarding/embedded-scripts.h> > > I disagree in this case since it's easier to understand what the > Makefile does. Offloading it to a seperate script file would mean we > have to deal with an extra script file without getting a benefit from it > (and compared to the monstrosity in Makefile.testshell this one is > small... :-) ). > > > usr/src/lib/Makefile.astmsg > > > > 50-51: yay! > > ?! > > [snip] > > usr/src/lib/libast/common/llib-last > > > > 129-945: this stuff doesn't belong here; properly-constructed 'llib' > > files should only need #includes -- the same #includes that > > the software linting against the library would itself have > > to use in order to get the interface definitions. Having > > giant lists of externs is a sign of internal design > > trouble. At a minimum, it means that the #includes are > > wrong. > > Erm... the #includes are Ok... I copied this stuff from another llib-* > file and always thought this is what "lint" wants... I assume the extra > prototypes can be removed, right ? > > [snip] > > usr/src/lib/libcmd/Makefile.com > > > > 128,132: mystery flag issue. > > See comment about -xcsi above... > > > usr/src/lib/libcmd/amd64/Makefile > > > > There are no substantive changes here; is this part of this > > integration? > > Yes... > > > usr/src/lib/libcmd/common/llib-lcmd > > > > 35-90: more design-level trouble. > > Erm... more likely dumb copy-stuff-without-knowning-what-it-does... see > above... > > [snip] > > usr/src/lib/libdll/Makefile.com > > > > 81,85: mystery flags. > > See comment about -xcsi above. > > [snip] > > usr/src/lib/libpp/Makefile.com > > > > 97,101: mystery flags. > > See comment about -xcsi above. > > [snip] > > usr/src/lib/libshell/Makefile.com > > > > 153,157: mystery flags. > > See comment about -xcsi above. > > > 160-161: what happened to require these new overrides? > > Substancial changes in the matching upstream sources. These should > disappear for the next update (I hope they don't pop-up elsewhere, > during the ksh93-integration update1 cycle lots of code was rewritten > and sometimes one warning was fixed and two new ones appeared. Next > update will fix these but I can't gurantee that something else will > show-up elsewhere), assuming the compilers on other non-Solaris > platforms "swallow" the fix. > > > usr/src/lib/libshell/Makefile.demo > > > > 143: what's this .WAIT do? It doesn't appear to be necessary. > > Originally doc/ had subdirs which are now gone. > Fixed (".WAIT" removed). > > [snip] > > usr/src/lib/libshell/common/llib-lshell > > > > 35-143: design trouble. > > Erm... more likely dumb copy-stuff-without-knowning-what-it-does... see > above... > > [snip] > > usr/src/lib/libsum/Makefile.com > > > > 73,77: ? > > See comment about -xcsi above. > > > 94-95: please remove. (Or at least get rid of the useless $(TRUE).) > > I only copied this from existing Makefiles which all seem to use this... > ... but I have removed the "$(TRUE)" thing. > > > usr/src/lib/libsum/THIRDPARTYLICENSE > > > > Not an issue for my review, but someone should make sure that this > > new license gets packaged and delivered properly. > > Uhm... what do you mean with that ? > > > usr/src/lib/libsum/THIRDPARTYLICENSE.descrip > > > > This description doesn't seem to match the license itself. The > > license itself seems to be "CPL" with IBM as a "steward" (serving > > soft drinks and peanuts, I guess). This description says AT&T, > > which appears nowhere in the license itself. What gives? > > Uhm... April: ping! > > AFAIK the license was invented by "IBM" and AT&T uses it... but the > other stuff is something only the legal folks can explain... > > [snip] > > usr/src/lib/libsum/common/llib-lsum > > > > 34-43: these lines should not be needed if the #include file set is > > correct. Is the #include here correct and complete? If not, > > can you fix it? > > See above... it's the copy-stuff-without-knowning-what-it-does disease.
ping! - Are the changes in http://cr.opensolaris.org/~chin/webrev-nonast.nov28-vs-dec4-diffs/webrev-nonast.dec4-diffs.patch and the one-liner in http://cr.opensolaris.org/~chin/webrev-nonast.dec4-vs-dec6-diffs/webrev-nonast.dec4-vs-dec6-diffs.patch Ok for you ? ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;)
