Roland Mainz writes:
> > > http://www.nrubsig.org/people/gisburn/work/solaris/ksh93_integration/ksh93_integration_prototype004_webrev_20070127/non_ast_files/webrev/
[...]
> > 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:
[...]
>
> 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) ...
OK; but that means that separate build rules aren't needed now, right?
> > 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.
OK.
> > 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
Part of the (seemingly unnecessary) complexity here is that
$(USRKSH_ALIAS_LIST) includes $(PROG), and this little script exists
in part just to skip over that one entry.
Perhaps it's just my preference, but I'd rather see traditional
makefile constructs in makefiles, rather than resorting to shell
scripts to do common things. And forcing $(SHELL) to something other
than /bin/sh seems doubly strange.
In this case, I probably would have done this in Makefile.com (note
that I've removed $(PROG)):
$(BINKSH_IS_KSH93)PROG= ksh
$(BINKSH_IS_KSH93)USRKSH_ALIAS_LIST=ksh93 rksh rksh93 pfksh
$(BINKSH_ISNOT_KSH93)PROG= ksh93
$(BINKSH_ISNOT_KSH93)USRKSH_ALIAS_LIST=rksh93
And then something like this in the Makefile:
INSLINKTARGET= $(ROOTPROG64)
ROOTLINKS64= $(USRKSH_ALIAS_LIST:%=$(ROOTBIN64)/%)
install: $(ROOTLINKS64)
$(ROOTLINKS64): $(ROOTPROG64)
$(ROOTPROG64): all
> 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... ;-( ).
We intentionally centralize things (such as the rules for installing
symlinks) to reduce duplication and so that when something needs to
change, we can be sure we're fixing all of the instances of it.
> 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...
I agree that it's not well-used. If you don't want to use it, I
suppose that's fine.
My reaction was mostly to the blob of shell code appearing in the
midst of a makefile. That's sometimes necessary, of course, but I'd
rather see it minimized if at all possible.
> > 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.
OK; thanks.
> > 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).
As above, I think the complexity here is in having $(PROG) appear
inside $(USRKSH_ALIAS_LIST), not the way in which the switch is made.
> > 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.
That's not what I was "yuck"-ing. My comment was on the mkdir -p
buried in here; it's very unmakefilish. (makefilian?)
I'd prefer something roughly like:
COBJS= \
sh/pmain.o
OBJDIRS= sh
$(OBJDIRS):
$(MKDIR) $@
$(PROG): $(OBJDIRS) .WAIT $(COBJS)
> > 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).
I'm unclear on the use of the phrase "seems to."
Does it indeed _require_ that ksh93 is on the path as "ksh," or does
it not require that?
> > 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.
Ah, ok. I misread where we were in the tree. You're right.
> > 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)).
I don't understand why this is needed at all. If it is needed, I
don't see how it can live with just the _minor_ version number, and
completely ignoring the major version number.
Moreover, we're only building for one version of Solaris here. It's
not possible for the code in the S11 tree to build S9 or S10 bits.
I'm not asking whether the upstream does something like this; I'm
asking _why_ it's done, and what purpose it serves.
> > 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,
What is `_ast_va_list', which is cited in the bug report? That looks
to me like an AST-specific implementation of varargs. Is it actually
something else?
> the AST code just has to
> deal with platform-specific varargs differences.
Why would it need to do that? Doesn't <stdarg.h> do the right thing
in every place that matters?
> 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.
There seem to be at least two other possible fixes here:
- Revert to using standards-based <stdarg.h> and abandon the
independent and likely unnecessary version.
- Get a fix for that lousy gcc "mkinclude" design flaw. It's awful,
and breaks things other than AST (including OS patches).
> 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).
Hence my confusion ...
> > 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 ?
The very next line (which follows with no whitespace) is this:
ROOTHDRDIR= $(ROOT)/usr/include/ast
I don't know what that comment has to do with that line.
> 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 ?
Yes, that's 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).
That sounds like a potentially serious design problem to me.
You also get those <sys/*.h> things just by using "ordinary" include
files, such as <stddef.h>.
> 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)).
I'm very leery of this. Not because I think _LP64 is the greatest
thing since sliced bread, but because I suspect it points to deeper
design flaws that will end up producing unstable builds.
How is it possible that AST is so dependent on system internals that
use if <sys/isa_defs.h> "screws-up" the compilation?
> > 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).
Then this is something that should be fed upstream: a split like that
is ugly, likely unnecessary, and will undoubtedly cause trouble in the
future.
That's the sort of thing I'd expect to find in SFW, not ON.
> > 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).
No, I don't mean collecting all the *.c files.
Instead, I might have build rules that look like:
objs/%.o: common/cdt/%.c
and an object list that says:
CDTOBJS= dtclose.o dtdisc.o dtextract.o ...
COBJS= $(CDTOBJS) $(COMPOBJS) $(DIROBJS) ...
> > 612,616: these shouldn't be necessary, I think.
>
> AFAIK they are required because the compiler doesn't create subdirs
> itself.
No, but your makefile should by using _targets_, not by invoking
"mkdir -p" several hundred times during the build.
> 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...).
It certainly adds complexity, and I don't see how it's really helpful.
> > 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 ?
Why would $(CPPFLAGS.master) be overriding anything you need for the
AST build?
I don't quite understand why this is an issue for AST in particular,
but not an issue for most other things in the ON gate. What's special
about AST here?
The only things I see in $(CPPFLAGS.master) (of any importance) are
-D_TS_ERRNO and -DTEXT_DOMAIN. Do those harm AST?
Do you need to assume that folks updating the ON software don't verify
and test their work? Or is it more complex than that?
> > 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.
I'd suggest rewording to explain what it is maintainers need to be
careful about. Something like: "do not change the __OBSOLETE__ value
without examining the symbols that will be removed, and evaluating
whether that breaks compatibility."
"Be careful" doesn't say enough. I'd expect _ALL_ people coding in ON
to be careful ... all the time. ;-}
> > 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... :-)
It takes about three seconds to check out the file and remove those
overrides.
> > 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...) ...
A const without initializer doesn't make any sense to me. What does
the code look like? (I don't seem to have access to it.)
> > 715-720: not really needed.
>
> It seems the build cannot live without that (e.g. "nightly" fails) ...
> ;-(
OK.
> > 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).
OK.
> 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 ?
Yes.
> > 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... ;-((
"README" is fine, if you feel the need.
In this particular case, this all looks like ancient history to me. I
don't see what lasting value it has.
> > 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
But why do they care?
> > 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... ;-(
OK.
> > 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.
The same applies for other compilation-time bits, such as lint
libraries.
> 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... ;-( ).
If someone is really playing around with the API, then the right thing
for that person to do is to select one of the following:
1. Build from source. This is open source code. Get the source
and use it.
2. Wait until the libraries are made into public interfaces.
Otherwise, you're just going to hurt yourself by looking into a
private (and presumably unstable) interface.
> > 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... :-)
I don't think they ought to be shipped.
> > 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...)
OK; no problem.
> > 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
OK.
> > Targetdirs
> > 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)) ...
OK. Sort of disturbing that it didn't fail, as that means there may
possibly be other cruft in that file.
--
James Carlson, Solaris Networking <[EMAIL PROTECTED]>
Sun Microsystems / 1 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
_______________________________________________
opensolaris-code mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
