[ Resolved issues removed. ]
> > > http://cr.grommit.com/~chin/ksh93-webrev-jun29/jun29-makefiles/
> >
> > I've completed my review of the Makefiles. Things are looking good --
> > most of my comments are quite minor. Nice work, Roland :-)
> >
> > General:
> >
> > * Seems like every AST-related Makefile contains:
> >
> > # Override this top level flag so the compiler builds in its
> > # native C99 mode. This has been enabled to support the math
> > # stuff in the AST tools.
> > C99MODE= $(C99_ENABLE) -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1
> >
> > # silence common libast&co. warnings (upstream will handle this
> > # later) ... ... about |#pragma prototyped| ...
> > CERRWARN += -erroff=E_UNRECOGNIZED_PRAGMA_IGNORED
> >
> > It would be really nice if we could centralize these somewhere.
> > (Just a request, not a requirement.)
>
> I had the same idea... but it would mean we would need an extra
> "include" statement and a new file extra for these flags... and that's
> IMO an overkill unless more "common" things could be abstracted somehow.
Well, another common thing would be the shared ksh/libshell "KSHCPPFLAGS".
Having a usr/src/Makefile.ast that has those three things seems worthwhile
to me. Yes, it's one more Makefile -- but I think it will ultimately help
maintainability and help us centralize the logic that bridges between the
lands of AST and ON.
> > lib/libc/{sparc,sparcv9,amd64,i386}/Makefile.com:
> >
> > * It's a shame this has to be duplicated across all four Makefiles,
> > but I guess that's status quo with the libc Makefiles.
>
> Well, we could declare the old version of |libc::wordexp()| obsolete
> (which will be an intersting ARC case because the current
> |libc::wordexp()| has seperate codepaths for normal and XPG4 behaviour
> (the difference is whether /usr/bin/ksh or /usr/xpog4/bin/sh is used)
> while the new one only supports XPG4 behaviour. The question is whether
> there is a difference between both shells when word expansion is done...
> I can't even imagine a testcase to probe for the difference between
> /usr/bin/ksh and /usr/xpg4/bin/sh and therefore I assume the difference
> is more or less "zero" (except that you could use shell-specfic
> variables and check $0 for the name of the shell - but AFAIK no consumer
> will do that...)) and this flag will disappear... :-)
OK :-)
> > * In theory, you need a FLG here so that when someone brings
> > over usr/src/lib/libc they'll get Makefile.ksh93switch brought
> > over too. But FLGs are dying soon with the Mercurial switch,
> > so I guess it doesn't matter.
>
> Uhm... what do you mean with "FLG" ?
A FLG is a TeamWare thing that causes bringover to grab additional files.
For instance, if building usr/src/cmd/foo requires source files in
usr/src/common/bar, then an "inc.flg" file in usr/src/cmd/foo would tell
bringover to also grab the stuff in usr/src/common/bar so that when
usr/src/cmd/foo is brought over. Then, the subsequent `make' in
usr/src/cmd/doo will "just work". In general, FLGs are needed whenever
you reference source files or Makefiles that are not along the path to the
directory you're building in. While FLGs will die with the rise of
Mercurial, the idea of "reaching over" to other "peer" directories is
still bad practice.
> > lib/libcmd/Makefile.com:
> >
> > * 104: "-Isrc/lib/libcmd" seems bizarre; why is this necessary?
>
> See
> http://mail.opensolaris.org/pipermail/busybox-dev/2007-April/000033.html
> for the layout of the upstream source files and includes. We basically
> use the same include file directory layout to avoid that we have to
> touch all consumers and adjust their "#include"-statements. This is one
> of the major items why the original "prototype001" failed - we didn't
> realise that the AST codebase has several include files with the same
> name in different subdirs with slightly different functionality - and
> getting the include order wrong may result in a code which compiles but
> won't work properly
I'm not sure I understand this yet. How do we choose which set of headers
with identical names get installed into the proto area? Do the functions
in those headers have identical names and signatures too? How does the
program choose which one it ends up linked against? Finally, what headers
are actually in the lib/libcmd directory as opposed to lib/libcmd/common?
> > lib/libdll/{sparc,sparcv9,amd64,i386}/Makefile:
> >
> > * GETRELEASEMINOR seems the hard way around; seems simpler to
> > update Makefile.master to have:
> >
> > RELEASE_MAJOR=5
> > RELEASE_MINOR=11
> > RELEASE=$(RELEASE_MAJOR).$(RELEASE_MINOR)
> >
> > ... then get rid of GETRELEASEMINOR. (But if for some reason it
> > needs to be kept, then please move it somewhere more common.)
>
> AFAIK this was discussed long ago. In theory you're right that such a
> thing belongs into Makefile.master ...
> ... but I am trying to avoid global changes for this putback. I am
> willing to fix this with a seperate putback (together with lots of other
> "janitor" stuff I've complained about in
> opensolaris-code at opensolaris.org) but please please not with this
> putback. Putting a new global flag into Makefile.master affects _all_
> Makefiles and I am not happy with having such a thing in the already
> large&&complex putback.
I'm a little surprised you're so scared of touching Makefile.master;
you've already touched other common Makefiles like Makefile.lib. I'd
(much) rather you did the right thing from the start, but as it's a fairly
minor thing, as long as you file a bug on it and fix it soon after
putback, I'll go along with it.
> > * Seems simpler to replace DLLPLATFORMCPPFLAGS with
> > DLLPLATFORM=sun4 (or i386), and then have Makefile.com have
> > "-DHOSTTYPE="sol$(RELEASE_MINOR).$(DLLPLATFORM)" in CPPFLAGS.
>
> Right... but DLLPLATFORMCPPFLAGS was modelled after a matching upstream
> build flag which is per-platform. I know it's currently slightly
> "redundant" but DLLPLATFORMCPPFLAGS is intended to contain only
> per-platform flags which are set in
> usr/src/lib/libdll/$(TRANSMACH)/Makefile ...
> ... your choice: Keep the current code or change it...
I'd like to see DLLPLATFORMCPPFLAGS die once you make the Makefile.master
changes.
> > lib/libast/{sparc,sparcv9,amd64,i386}/Makefile:
> >
> > * Same comments as libdll above.
>
> See comment above...
... and same response :-)
> > * 134-171: Could we keep the CPPFLAGS in cmd/ksh/Makefile.com
> > and here in-sync through includes instead?
>
> Uhm.. in theory yes... but it would generate "yet another" Makefile
> include and IMO the current four/five/six-level hieracy is mind-bending
> and sometimes it's already tricky to answer the question "...where does
> _that_ weired flag come from..." (somehow I wish there would be
> something like a "makefile debugger").
> Erm... your choice... keep current code or create seperate Makefile
> include (I would prefer to keep the current makefile code, assuming that
> people read the comments in the makefiles... :-) ) ...
Let me know what you think of the usr/src/Makefile.ast suggestion above.
> > * 143: -Isrc/cmd/ksh93 seems suspicious; why are header files
> > for the library lurking over in a command directory?
>
> See above... it's the upstream source file layout.
Can you be more specific in this case? What header files is it taking
from the src/cmd/ksh93 directory? Assuming that those header files
contain functions, where do the implementations come from? (You need to
ensure the answer is never "the build machine's /lib or /usr/lib".)
> > cmd/Makefile:
> >
> > * 31: Why include ksh/Makefile.ksh93switch here?
>
> Originally it was added for the /sbin/ksh93 machinery which was removed
> during PSARC 2006/550... AFAIK April had some ideas to (later) turn-off
> putting ksh88 into /usr/bin/ksh when the flag is "on"...
> ... April ?
>
> > If we really
> > need this, seems like Makefile.ksh93switch needs to move to a
> > more common directory.
>
> Any suggestions where I should it move to (AFAIK usr/src/, right ?) ?
Yeah. But only if April can explain why it's needed :-)
> > cmd/ksh/Makefile:
> >
> > * 65-74: I'm confused why this logic is here and at e.g. lines
> > 39-46 of cmd/ksh/i386/Makefile.
>
> This logic is needed to install isaexec links for all variants of ksh
> (e.g. restricted shell, profile shell etc.) depending on the setting in
> Makefile.ksh93switch and the values of USRKSH_ALIAS_LIST and PROG,
> ...
I think I understand what the logic is for. What I don't understand is
why the *same* logic appears to exist in cmd/ksh/Makefile,
cmd/ksh/i386/Makefile, and cmd/ksh/sparc/Makefile.
> > cmd/ksh/Makefile.com:
>
> > * 57-58: See previous comment on the need to share this
> > list with libshell.
>
> See above... IMO "yet another" makefile include will make things
> worse...
See my reply above.
> > * 132: I think this comment is supposed to explain why we delete
> > ksh and ksh93 even for `clean' (rather than `clobber'), but I'm
> > not quite getting it.
>
> We delete _both_ targets to make sure that a change in
> "ON_BUILD_KSH93_AS_BINKSH" gets handled properly. It's a safeguard to
> avoid that anyone works with stale binaries by accident (and there is
> the old issue with "io.sh" (which is AFAIK fixed but I didn't had time
> to verify that yet... ;-( )).
But the `clean' target is not supposed to remove the final products of an
`all'; that's what `clobber' is for.
> > cmd/ksh/Makefile.testshell:
> >
> > * 38-44: So will this test suite failure be resolved prior to
> > integration?
>
> It doesn't occur for the "C" locale, only some of the non-"C" locales
> are affected and I strongly belive it's only the sort order of the
> locale. I've looked at the problem and IMHO it's not important for now.
OK.
> > * 56-61: Since we're well past build 64, can this issue be
> > removed? (If not, please reformat the comment to be 80-column
> > friendly.)
>
> Erm... I don't have a build machine > B61 and I can't simply update my
> two build machines because I would either loose the abilty to test the
> |libc::wordexp()| code or have to update the whole tree to a newer
> Nevada build (which would trigger problems for April because she would
> have to re-do her whole SCCS tree...).
But you're well aware of the issue, so the comment is not for you, right?
Others will be running on newer builds (since B61 is history), so I don't
see why the comment is needed.
> > * 113-143: Please reformat to be 80-column friendly.
>
> OUCH... ;-(
> Is this really neccesary ?
Yes; our Makefile style rules require an 80-column margin. (There are
some violators; they are a hazzard.) If you're concerned about
readability, could the whole thing be moved out to a standalone script
which takes some command-line arguments via a Makefile invocation? As it
is, it's already pretty hard to read IMHO :-(
> > lib/Makefile.astmsg:
>
> > (I'm also unclear why ASTMSGCCFLAGS
> > is needed, rather than directly using CPPFLAGS.)
>
> Erm, if I put the values directly into CPPFLAGS it would affect the
> normal *.c ----> *.o rules. ASTMSGCCFLAGS is seperate to have a way to
> 1) monitor the values which are "special" for msgcc and 2) set some
> values which are not set in CPPFLAGS/CFLAGS.
I see.
> > * 69, 63, 76: Why is CFLAGS needed?
>
> "msgcc" works like the normal compiler and should get the normal
> compiler flags, msgcc will then process the matching sources like "cc"
> and finally extract the l10n strings.
I see.
> > * 90: $(TOUCH) instead?
>
> I am using "touch" to create an "empty" message catalog file for now
> when we don't build the AST message catalogs, e.g. if ...
My question was about why you're not using the $(TOUCH) macro, not about
why you're using `touch' conceptually.
> > cmd/sgs/libelf/Makefile.com:
> >
> > * Not sure why the DIRMODE setting is no longer needed.
>
> AFAIK it was the same as the default mode.
I see.
--
meem
