James Carlson wrote:
>
> Roland Mainz writes:
> > * Webrev over all non-AST files (this includes the files in
> > usr/src/cmd/ast/msgcc/ my accident):
> > http://www.nrubsig.org/people/gisburn/work/solaris/ksh93_integration/ksh93_integration_prototype004_webrev_20070127/non_ast_files/webrev/
>
> I did some light review of this. Some comments:
>
> cmd/ast/msgcc/Makefile
>
> 56-66: why are these needed? Don't the default targets work
> properly? The usual way to do this would be:
>
> PROG= msgcvt msggen msgget msgcpp
>
> and let make figure out the implicit dependencies. Is there some
> reason that doesn't work here?
Not really... the Makefile was a patchwork hacked together at 5:00AM
from other bits of the tree (and it seems I collected all the
imagineable bugs, too... ;-( ).
Note that the ASTPROG was used because I expect that the AST* rules move
to the Makefile fragment which run the normal+XPG[46]* rules some day
(if we get more AST tools in the tree) ...
> 69-75,78-83: consider creating a common build rule for these.
Fixed.
> 91-92: what is this? This ought o be a part of the Targetdirs.
Ouch...
... Fixed.
> 99: I'd suggest a dependency rather than .WAIT.
Fixed (or better: removed since the directory creation was moved to
Targetdirs).
> 105-110: why is this needed? What's wrong with the default rules?
Nothing except that I used the custom rules for debugging.
See bug #143 ('RFE: Need some cleanup in/for "SUNWastdev" package' -
http://bugs.grommit.com/show_bug.cgi?id=143) for the patch which cleaned
these parts up.
> cmd/ksh/amd64/Makefile
> cmd/ksh/i386/Makefile
>
> 39-48: use the $(INS.link) rule instead.
Erm... that won't work AFAIK. $(USRKSH_ALIAS_LIST) is a variable list
and changes based on the switches defined by "Makefile.ksh93switch" (we
support switching the flag in one build to do testing from both sides
without requiring a full rebuild from scratch (remember my fastest build
machine at home needs 8-10 hours to do one build and the slowest chews
almost two days on the tree... ;-( )). I would prefer this solution
since it provides a central place to control the deployment of links and
binaries to avoid having too complex Makefile machinery in too many
places (I've shot myself too often into my feet with this kind of the
Makefiles... ;-( ).
BTW: Is it possible that INS.link is one of the orphans in the tree ?
http://src.opensolaris.org/source/search?q=INS.link&defs=&refs=&path=%2Fonnv&hist=
only lists two consumers tree-wide...
> cmd/ksh/Makefile
>
> 58-61: why is i18n dummied out?
/usr/bin/$(MACH)/ksh93 is just a frontend which directly calls into
libshell.so.1. There is no l10n code... it's all in libshell.
I added a comment in the Makefile.
> 66-73: looks like $(INS.link).
Same problem as in usr/src/cmd/ksh/$(MACH)/Makefile:
$(USRKSH_ALIAS_LIST) is a variable list and the tree supports switching
the install mode defined by Makefile.ksh93switch between single builds
for testing (otherwise a rebuild from scratch may be required).
> cmd/ksh/Makefile.com
>
> 99-101: why is a custom build rule needed? Why not just set up a
> dependency?
Cloned from the libshell makefile (for the mkdir). See below...
> 100: yuck.
The placement of object files follows libshell/libast/etc. where all
object files are stored in subdirs. This is done for consistency in this
case since the "ksh" frontend is in fact a part of the libshell sources
and uses (almost) identical build flags.
> 105,120: s/expiclitly/explicitly/
Fixed.
> 108: "seems to require"?
See usr/src/cmd/ksh/Makefile.testshell, the comment about "io.sh"
(originally it was part of usr/cmd/ksh/Makefile.com which would be much
clearer in this case).
> 126-131: this is what Makefile.lint is about; just remove cmd/ksh
> from the COMMON_SUBDIRS list (it's not there now, by the way), and
> all should be fine. This extra bit of work shouldn't be needed.
Uhm... last time I tested this the tree didn't work without it.
> lib/libast/Makefile
>
> 159: I think you might want just "FRC" here, though it's not clear.
> The current build rule will cause make to execute the $(SUBDIRS)
> build rule with an empty $(TARGET) -- building whatever is the
> default target in $(MACH).
Fixed (in lib/lib(ast|dll|pp|cmd|shell)/Makefile).
> lib/libast/i386/Makefile
>
> 34: what is this about? What do Solaris distributors need to do
> when updating the minor release number? (And why doesn't the major
> release number fit in here somewhere?)
That is a #define used by the native AST/ksh93 build to set some
version-specific stuff. The native AST/ksh93 build sets
HOSTTYPE="sol9.i386" for Solaris 9, HOSTTYPE="sol10.i386" for Solaris
10, HOSTTYPE="sol11.i386" for Solaris 11 etc. and I cloned the behaviour
in the OS/Net Makefiles that we do not have to maintain such values
(AFAIK Mike suggested that we should do that automatically;
unfortunately the OS/Net Makefiles do not provide this value which
forces us to calculate it somehow within our own Makefiles (not perfect
but the value is more or less only used for informational purposes and
won't cause puppy heads to explode when it's wrong)).
> 47-60: what's the status of the real fix? (I don't really follow
> why this is a special problem for ksh93 and doesn't seem to affect
> anything else in ON -- all of which, varargs and not, goes through
> gcc shadow compilation. Is the bug actually related to the new
> AST-specific varargs?)
Erm, there are not any AST-specific varargs, the AST code just has to
deal with platform-specific varargs differences. We generate the AST
includes by building AST/ksh93 the "normal" way (via buildksh93.ksh) and
then copy all the generated includes in our tree. The problem is that we
use Sun Workshop/Forte/Studio as compiler - and in this case we run into
a tiny problem since gcc defines it's own flavor of varargs which the
headers do not recognize because they "think" they're dealing with Sun
Workshop/Forte/Studio. We can workaround the problem in two ways: Edit
the generated AST includes or just overriding the defines from "outside"
via the Makefiles.
I strongly perfer the 2nd method because the AST includes are machine
generated (as described above) and each update just "stomps" over the
old includes (plain copy). Local modifications would therefore be very
painfull (e.g. they would need to be documented, maintained and tested
somehow) and a source of constant build/maintaince problems for each
update.
The real fix is to upgrade to gcc4.0 and/or throw stones after the gcc
people to stop them from using their own varargs header version (both
Sun Workshop/Forte/Studio and gcc varargs are interoperable from the
binary interface, they just use different includes which causes the
hiccups in this case).
> lib/libast/Makefile.astinclude
>
> 28-29: is this true?
Yes, that's true... the headers are partially machine-generated based on
"iffe" probes ("iffe means "if feature exists"... like "autoconf" on
steroids) in a native AST/ksh93 build (see above) and then copied over
into the OS/Net tree.
> (And if it is, how does the comment help
> explain the code that follows?)
Uhm... what do you mean in this case ? It's a general explanation (BTW:
the comment "i386, sparc" predates the introduction of the 64bit AST
libraries in our tree)...
... I changed this to:
-- snip --
# ident "%Z%%M% %I% %E% SMI"
#
# Note: libast headers are generated by the AST build system outside
OS/Net
# (and then copied here) and depend on the architecture (e.g. "i386",
"amd64",
# "sparc", "sparcv9" etc. ...), we later merge them into one unified
file
# (see below)
ROOTHDRDIR= $(ROOT)/usr/include/ast
-- snip --
Better ?
> 32-38: I don't follow this. What exactly is wrong with _LP64? It's
> set correctly by sys/isa_defs.h -- why would you need to do your own
> version of this logic?
The problem is <sys/isa_defs.h>. It's a system include in the sys/
directory. The Sun Workshop/Forte/Studio compiler does not define
|_LP64| which means it's not available unless you include system
includes which interfers with the AST includes ("interfers" is an
understatement... it completely screws-up the AST headers which depend
on precise include order of it's own and the system includes). After
lots of trouble we've settled with avoiding the usage of |_LP64| since
it's not portable enougth (and using -D_LP64=1 at compile time does not
work either (it compiles but the resulting code will be garbage)).
> 40: What's the justification for having include files that are
> dependent on architecture? This seems too complex by at least half
> to me.
See above. The includes are partially machine-generated and we just copy
them over However the "iffe"-probes only work for the architecture for
which they are compiled for and the design does not anticipate that one
OS has two different architectures, e.g. 32bit SPARC and 64bit SPARC. To
solve this problem we just compile the native AST/ksh93 one time for
32bit and one time for 64bit and copy the generated includes over into
the matching directories (even retaining the directory layout for the
generated includes to make maintaince as simple as possible). Finally we
use "diff -D" to create one unified set of include which are then
installed in /usr/include/ast/ (the differences between 32bit and 64bit
Solaris includes are usually not very large but the current approach
gurantees that we don't run into any problems by accident and keeps the
maintaince/upgrade requirements at a minimum level).
> 56: s/concaternation/concatenation/
Fixed.
> lib/libast/Makefile.com
>
> 33-602: suggest condensing this list with appropriate build rules.
Erm... what do you mean with "appropriate build rules" in this case ?
Using pattern-matching to collect all *.c files won't work (not all *.c
files are directly compiled or used within the libast code directly).
> 612,616: these shouldn't be necessary, I think.
AFAIK they are required because the compiler doesn't create subdirs
itself. Remember my original posting for this wevrev - we put the *.o
files into subdirs to avoid having a few hundred *.o files in one single
directory... IMO the current solution is much more tidy (I wish libc
would use the same method...).
> 646-650: I don't really follow. Can this be reworded?
Basically we're worried that changes in $(CPPFLAGS.master) may cause
future silent breakage. Currently $(CPPFLAGS.master) is applied after
the content $(CPPFLAGS) when building the compiler's command line. If
anything in $(CPPFLAGS.master) overrides any "-D" symbols needed by the
AST build we're in serious trouble (we had such a case in the past and
debugging the issue was NOT fun... ;-( ).
Any suggestions for rewording ?
> 650: s/expliclity/explicitly/
Fixed.
> 653: what does "be careful" mean here?
Putting the wrong value here can "EOL" older APIs in libast which may
still be needed. This does not affect our libast in OS/Net but the AST
codebase in general, e.g. tools build against libast binaries build from
upstream should work with our libast if possible.
> 653: s/carefull/careful/
Fixed.
> 682-708: not sure that the comments add much value here.
Well, they are at least usefull for me that I know where I have to look
at... :-)
> 709: that's worrisome.
Uhm... I don't see anything obviously wrong in this code, the code works
and neither "dbx -check access" or "Rational Purity" complain (nor does
gcc) .. therefore I've opt'ed to silence this warning and add it into my
ToDo list for later (somewhere after dealing with this putback,
|posix_spawn()| and "shcomp"&&friends...) ...
> 715-720: not really needed.
It seems the build cannot live without that (e.g. "nightly" fails) ...
;-(
> lib/libast/Makefile.libastl10n
>
> 36-37: this has to go. We can't support LD_LIBRARY_PATH hacks that
> attempt to run software out of the proto area.
We know... that's http://bugs.grommit.com/show_bug.cgi?id=118 ("Disable
the usage of tools created during the OS/Net build (ksh93, msgcc
etc.)"). We save the disabling of this code until the last moments
before the official review to make sure we don't break that (we have to
disable it, wait until SUNWastdev is deployed everywhere and then
enabkle this code again without the LD_LIBRARY_PATH hackery).
I added the following comment section to the file:
-- snip --
# WARNING: This has to go (see bug #118 # ("Disable the usage of tools
# created during the OS/Net build (ksh93, msgcc etc.)" -
# http://bugs.grommit.com/show_bug.cgi?id=118)).
# We can't support LD_LIBRARY_PATH hacks that attempt to run software
# out of the proto area.
-- snip --
Better ?
> lib/libast/mapfile-vers
>
> 29-618: unhelpful comments; find another place to document the
> history.
Where should I put this information ? The *.diff file, touristguide.xml
etc. are currently under the "we don't want/have/tolerate such files
(kill them!! burn them!!) in the tree"-attack, remeber ? AFAIK we have
to settle that discussion to find a (new ?!) place where the
putback-specific documentation can be stored... ;-((
> lib/libcmd/i386/Makefile
>
> 35: please don't add stray blank lines at the end of the file.
Fixed for lib/libcmd/(i386|amd64|sparc|sparcv9)/Makefile
> lib/libcmd/sparcv9/Makefile
>
> 28: not sure why this is needed.
All Makefiles used for libast/libpp/libdll/libcmd/libshell/etc. use
SHELL=/bin/ksh
> lib/libc/port/regex/wordexp.c
[snip]
I'll move the workexp.c parts into a seperate email thread... I guess
may become a little bit longer... ;-(
> lib/libshell/misc/buildksh93.readme
>
> 33: s/it's/its/
Fixed.
> 71: missing "##"?
Fixed.
>
> pkgdefs/SUNWarc/prototype_com
>
> 75-76,113-114,182-183: something seems out of sync here. If you're
> not shipping the compilation symlinks (the lib*.so symlinks), then
> why ship the lint libraries? How is linting source that cannot be
> linked useful?
> (See SUNWcsl; it has only the *.so.1 files, not *.so links.)
Known issue. Solaris has the strict rule that we're not allowed to ship
any *.so files if the API is not public, therefore we do not ship the
*.so files. However some people may be interested to play around with
the API (e.g. to build ksh93 plugins (builtin commands, functions etc.))
... the *.so links can easily be restored by hand but the lint files
cannot be "restored" if we don't ship it. That's why we're ending up
with a weired chimera shipped with three legs where leg No. 4 can be
substituted with a wooden one on demand (erm... Ok... this metapher
isn't a good one... ;-( ).
> pkgdefs/SUNWarc/prototype_i386
> pkgdefs/SUNWarc/prototype_sparc
>
> As above; can't tell why lint libraries are shipped.
See above, the three-legged chimera. Basically we honor the rules and
still try to keep the value alive somehow... :-)
> pkgdefs/SUNWastdev/depend
>
> Curious: how was this list determined?
Oh-oh... OH-OH... *duck*...
... seems this was done by turning off the brain, copying the package
files from one of the perl packages in pkgdefs/ and then thinking I can
get away with this thing. Fixed with
http://bugs.grommit.com/show_bug.cgi?id=143 (yes, this was really
dumb...)
> pkgdefs/SUNWastdev/Makefile
>
> 32: you have your own 'depend' file. There shouldn't be a
> dependency on it here! Please remove, as it'll cause source-only
> builds to fail.
See above. Fixed with http://bugs.grommit.com/show_bug.cgi?id=143
> pkgdefs/SUNWcsu/prototype_i386
>
> 106-107: alphabetic order, please.
Fixed.
> pkgdefs/SUNWcsu/prototype_sparc
>
> 55-56: alphabetic order.
Fixed.
> pkgdefs/SUNWhea/prototype_com
>
> 165-166: I think 's' comes before 't', so 'assert.h' comes before
> 'ast'.
Fixed.
> pkgdefs/SUNWosdem/prototype_com
>
> 60-99: alphabetic order, please.
Fixed.
> Targetdirs
>
> With the exception of line 223, all of the new entries in this file
> look wrong to me. The "REALPATH" goop is needed for libraries that
> are in /lib (in the root file system). That's not true of *ANY* of
> the new AT&T libraries. They're all over in /usr/lib. Thus, no
> magic "../../lib/" symlink (referring to the root file system) is
> needed.
Umpf... that's an artifact of the part which got ripp'ed out during
PSARC 2006/550 where the original plan was to target delivery into /lib.
Somehow this was sacrified (at the expense of having another ARC case
just to move that back again to /lib "on demand") and all the ksh93 libs
suddently found themselves in /usr/lib instead... ;-((((
Fixed.
> How does the software even build with these rules present? (It
> should fail ... but I guess it's possible that there are bugs that
> prevent it from failing ...)
AFAIK it only creates the links and nothing in the tree (including
"makebfu") tests whether links are correct or not (and the build rules
use $(RM) to remove targets before writing new files (and that removes
these links)) ...
----
Bye,
Roland
--
__ . . __
(o.\ \/ /.o) [EMAIL PROTECTED]
\__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer
/O /==\ O\ TEL +49 641 7950090
(;O/ \/ \O;)
_______________________________________________
opensolaris-code mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
