Roland Mainz writes: > > 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" ?
"Tender Loving Care" -- it means that I think there are some deficiencies in how the libraries are constructed. If these are known issues that will be addressed later, then that's fine by me. If they're not known, then I want to bring them up now. (See below.) > > 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. None of that should really be a barrier. I guess I don't care too much about this, and you can certainly plow on without changing it, but I do think can make the source slightly harder to manage over the longer term, because you may end up with a small forest of confusing "-I" options. > > (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. As I was trying to point out, we already do put private stuff in $(ROOT) if it's used in the process of building multiple bits in the system. To keep it private, we just don't package it. > > 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. The standard ON practice is not to change files unless there are actual changes that need to be made. For unchanged files in the gate, both forms of CDDL are still acceptable (though one is obviously preferred), which is why you're not seeing anyone else rushing to change all of them. And since no substantive changes have been made, the copyright date should _not_ be changed. (That may actually be a "must not" -- last time I checked, it was a legal and ON gatekeeper issue. Notably, when I added CDDL in the first place, I wasn't allowed to change any of the dates.) If you really want to mop up any old-style CDDL files, I suggest doing it as a separate integration, and perhaps attacking the whole gate at once rather than just doing a few ksh93-related ones. Otherwise, the existing "when there are other changes to be made" policy ought to be fine. > > 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. Wow; that's a strange one. OK. It'd be nice to have either a comment here or a CR filed so that future generations won't find this odd flag and either (a) rip it out because they don't understand what it does or (b) go all cargo-cult on us and copy it into other makefiles. > > 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. Oh! OK; it's a non-trivial change in that case. > > 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. OK ... it still seems odd to me, as "restricted shell" is all about nailing down the user's access and a "profile shell" normally grants extra privileges. Have you discussed the combination with any of the RBAC folks? The idea seems (to me) to confuse the separate notions of "user" and "role." Existing restricted shells typically aren't used for scripts -- they're assigned to user accounts to restrict what those users do. Profile shells, by contrast, typically aren't assigned to login accounts -- they're invoked in scripts or by users from within other shells. I suppose it's possible to combine these two for a very special user, but it'd be interesting to see some concrete usage scenarios, particularly ones that (in some way) work better with this combination shell. In any event, that CR you're citing is in "Dispatched" ("nobody cares") state; it'd be good to have someone add an evaluation and get it into a state where you can integrate. (Yes, I know that's something you can't control directly ...) > > 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. Sure; makefile variables set to "#" are the way we usually manage conditional makefile structures. > > 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... :-) But why is it needed both in the top level makefile and in this makefile.com? Are both this makefile and the subdirectory makefiles producing the same targets? If so, why? Doesn't that just mean that we do the same work twice? > 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. OK. > > 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. I guess I don't see how that happens here, but OK, I'm fine with that. > > 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... :-) ). I agree with that part. ;-} > > usr/src/lib/Makefile.astmsg > > > > 50-51: yay! > > ?! I'm very happy to see the LD_PRELOAD go away. > > 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 ? Yes. The #includes should almost always specify the correct set of external symbols for the libraries, so they should be the only thing included in the llib-* files. This is the "TLC" issue from the top level. The exception to this occurs when there's some fancy (or ugly) #ifdef or #pragma work going on in the header file such that not all of the necessary symbols are actually defined there. Those cases should be quite rare in practice, and I don't see that they apply here. > > usr/src/lib/libshell/Makefile.com > > 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. OK; as long as you're on top of them, and they'll be removed. > > 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 ? The license bits have to show up in the 'copyright' files for the packages produced. I haven't checked whether that's done correctly here -- if it is, then no problem. I'm just warning that it's an issue to check (and outside of makefiles). > > 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... OK; I _think_ I understand this now. I was a bit thrown by the apparent differences between the license and the description of it. -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
