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. ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;)
