Roland Mainz wrote:
> Hi!
>
> ----
>
> I created a couple of webrevs in various flavours based on today's
> ksh93-integration sources. This isn't the "reliminary review request"
> (yet; the official rquest should come on ~~ monday or tuesday), just an
> attempt to provide an updated webrev.
>
I'm yet to look in any detail at the webrevs, but most of the comments I had
from looking at diffs prior to this are covered in your notes, so I'll
comment on them.
> ** Notes:
> - All AST sources are in usr/src/lib/lib(ast|pp|dll|cmd|shell)/common/
> and usr/src/cmd/ast/. All files are 100% identical with the upstream
> (AT&T) versions except those listed in
> "usr/src/lib/libshell/misc/ksh93_solaris_builtin_patch.diff".
> ####----> All changes to the upstream sources ====> MUST BE <==== stored
> in *.diff files (and a README per *.diff) in usr/src/lib/libshell/misc/
> to make sure they can be backed-out and re-applied when the AST sources
> in the OS/Net tree get updated (e.g. the usual update procedure works
> like this: diff old and new upstream sources, backout the diffs from
> usr/src/lib/libshell/misc/*.diff from the OS/Net tree, apply diffs from
> upstream, reapply diffs from usr/src/lib/libshell/misc/*.diff, refresh
> files in usr/src/lib/lib(ast|pp|dll|cmd|shell)/(${MACH32}|${MACH64})/
> from a native AST build).
I don't understand why this is necessary, the SCM will provide you with
diffs and their associated comments.
> Anyone who is patching files in
> usr/src/lib/lib(ast|pp|dll|cmd|shell)/common/ and usr/src/cmd/ast/
> without storing diffs+READMEs in usr/src/lib/libshell/misc/*.diff will
> be <insert-some-horrible-torture-here (gatekeepers should decide how the
> punishment should look like... I'd suggest the usage of deep pits filled
> with komodo dragons, hot iron, boilling acid etc. (in random order))>.
Has this diff dance actually been agreed upon by anyone? I'd like to see an
explanation of why you can't just use the SCM to fulfill your needs. (I can
accept one may exist, I just don't see what it is...)
> - The includes delivered to /usr/include/ast/ are a merge of 32bit and
> 64bit includes from
> usr/src/lib/lib(ast|pp|dll|cmd|shell)/(${MACH32}|${MACH64})/include/ast/
> created using "/usr/bin/diff -D". This allows the usage of one unified
> set of includes for 32bit and 64bit binaries instead of shipping two
> different copies.
I think someone else commented on this so I'll keep out pending that
explanation...
> - usr/src/lib/lib(ast|pp|dll|cmd|shell)/Makefile.com list "-erroff="
> flags per object file, not globally, resulting in lists which look
> "huge" because we list each appeance of a suppressed warning explicitly
> including a comment (and "yes", we're working on reducing the list, step
> by step).
I don't see why this can't be done prior to putback, beyond your aversion to
deviating *at all* from the upstream sources. I'd far prefer to see these
lists empty prior to putback, even if upstream haven't accepted patches (yet).
-- Rich
