James Carlson wrote:
> 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.)
I know we have some deficiencies, including compile warnings, lint
cleanness, real bugs in the code and other hair-raising stuff. We're
working on it constantly and are trying to cleanup some of these issues
with "ksh93-integration update2" which should follow really soon (maybe
even in January but hopefully February as latest date ; the plan is that
ksh93-integration update2 "mops-up" any issues caused by update1 and
doesn't include an insanely large set of new features like this update
(or better: All features envisioned for update2 are already present in
the update1 codebase and only need to be ARC'ed, beyond that we only do
bugfixing)).
But for now I really wish we can do the putback for update1 _ASAP_ since
we're now many many months late because we tried to fix any bug+RFE
filed and prepare a "hyper-perfect" putback (which backfired badly, we
even missed the OpenSolaris 2008.11 release(!!) which was _extremly_ bad
and caused lots of "collateral damage" elsewhere).
> > > 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.
Well, I disagree a bit since IMO it only adds some complexity in the
Makefiles without having a benefit from it (e.g. OS/Net doen't support
partial download+compilation or something like that). We can try to
remove the usage of private headers from the "shcomp" and "ksh" frontend
sources (which would eliminate the cross-tree include linking in this
case).
> 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.
Well the forest is unlikely to grow in this case since we have the only
two consumers ( { ksh, shcomp } ) of the private headers in the tree.
[snip]
> > > 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.
Ok... I reversed usr/src/Makefile.ksh93switch to the current version ?
> Notably, when I added CDDL in the first place, I wasn't
> allowed to change any of the dates.)
Why ?
> 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.
Ok... but the original idea was just to update the AST+ksh93 CDDL
Makefile bits and not everything under the moon...
> > > 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
Already fixed - I centralised the CFLAGS/CFLAGS64 bits in
usr/src/Makefile.ast and added a comment there. The bug report will
follow after the integration that we can simply say "... remove that
flag in OS/Net and watch your compiler die in a horrible way...".
> 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.
"cargo-cult"... ROTFL... :-) :-)
[snip]
> > > 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 --
Uhm... that's not true. If you look at the demo section you'll see some
stuff like the IRC bot which switch to restriced mode to process
user-input (to prevent any possibilties that the user input can be used
to compromise the host machine of this internet-facing application).
> 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.
>From my experience you sometimes have tasks like _manually_ starting a
backup by employing students (or in my case it was part of my "Civilian
Service" (see http://en.wikipedia.org/wiki/Zivildienst) to go once a
week to a Himalaya-class system and insert tapes, run backup (and wait
the 30-90mins in the room until the backup is done), remove tapes, store
them in the next room). And in such cases they should only be allowed to
execute _exactly_ the commands they are supposed to do and not use the
machine to download, compile and run Quake2.
That's why I think a "pfrksh" is a good idea (I disagree a bit about the
abuse of students for that but that seems to be the way this world
works... :-) ).
> 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 ...)
Well, I was temped to request sponsorship for this RFE but it's for the
original Bourne shell codebase and I have sworn myself not to try to fix
this thing - it would be a sisiphos-type of work (that starts with all
multibyte handing bugs in this code, crashers, problems with memory
allocations, signal handling issues and other stuff).
> > > 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.
In the meantime I didn a small experiment which quickly grew out of
control - or better "out of linecount": Right now it's around 59 lines
and I have problems bending my mind to understand how it works. So the
correct answer is more likely: It is possible but may cause brain-damage
to the author. I really prefer my current version since I can at least
understand it (and explain it to anyone who needs an explanation) by
looking a few seconds on it.
> > > 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?
No, they are seperate targets because we use "isaexec" to select the
32bit/64bit versions (and in the future machine-optimized versions may
appear since we now have proof on x86 that we get a nice performance win
when we compile some of the libs with more advanched options which will
not work on older CPUs).
For example the curernt layout on x86 looks like this:
-- snip --
/usr/bin/ksh93 <-- isaexec
/usr/bin/rksh93 <-- hardlink to /usr/bin/ksh93
/usr/bin/i86/ksh93 <-- 32bit frontend binary
/usr/bin/i86/rksh93 <-- hardlink to /usr/bin/i86/ksh93
/usr/bin/amd64/ksh93 <-- 64bit frontend binary
/usr/bin/amd64/rksh93 <-- hardlink to /usr/bin/amd64/ksh93
-- snip --
Based on the switches this layout may change.
[snip]
> > > usr/src/lib/Makefile.astmsg
> > >
> > > 50-51: yay!
> >
> > ?!
>
> I'm very happy to see the LD_PRELOAD go away.
Ah... we now have /usr/bin/ksh93 available on all build machines (it's
even guranteed since "bldenv" and other things now depend on it) - the
LD_PRELOAD was therefore no longer needed. The same will apply to
"shcomp" in 3-5 builds after this putback.
> > > 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.
Ok... originally I cloned the AST llib-* files from other llib-* files
and thought "lint" requires to explicitly list the prototypes in the
base file (which contradicts a bit how cpp/cc should work but I forgot
to ask about this weiredness).
> 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.
Ok...
> > > 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.
Yep, we're trying (the "trying" is somewhat related to the issue that
AST+ksh93 run on other platforms with other compilers and getting them
all under one hat is not easy (yes, yes, I know... OS/Net policies don't
care about cross-platform portabilty... ;-( )) to get rid of them (but I
can't gurantee to kill them all in one single step nor that new ones
will grow... ;-( ).
> > > 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).
How can I check 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...
>
> OK; I _think_ I understand this now. I was a bit thrown by the
> apparent differences between the license and the description of it.
Ok...
----
Bye,
Roland
--
__ . . __
(o.\ \/ /.o) roland.mainz at nrubsig.org
\__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer
/O /==\ O\ TEL +49 641 3992797
(;O/ \/ \O;)
