>>>>> "Roland" == Roland Mainz <roland.mainz at nrubsig.org> writes:
>> * cmd/ksh/Makefile.com >> - why is GROUP set to root? [...] >> The current Solaris ksh88 is installed as group bin. Roland> Fixed (see http://polaris.blastwave.org/changeset/294). Looks good. >> * usr/src/cmd/ksh/Makefile.com >> usr/src/lib/libshell/Makefile.com >> - CPPFLAGS: it would better to have a single makefile fragment for >> -D/-U that is included by these two makefiles Roland> I thought about that - but the usr/src/cmd/ksh/Makefile.com Roland> AFAIK differes in the -I statements used since it does not have Roland> to reference libshell's internal sources. Your choice: Should I Roland> add a common "Makefile.libshellflags" for these options or leave Roland> it in it's current status ? I'd like to see a makefile fragment that just has the stuff that needs to be the same. Maybe it could be called Makefile.libshellcommon? Define one or more variables that have the common settings in that makefile. Then have the ksh and libshell Makefile.com's include it and build up using the defined variable. Skeleton example: Makefile.libshellcommon: COMMONDEFINES = -DKSHELL -DSHOPT_APPEND ... ksh/Makefile.com: include $(SRC)/lib/libshell/Makefile.libshellcommon [...] CPPFLAGS = $(DTEXTDOM) ... $(COMMONDEFINES) ... (At least I hope something that simple will work.) >> * usr/src/cmd/ksh/Makefile.com >> - install target: why add the "set -x"? Roland> From ksh(1): Roland> -- snip -- Roland> -x Prints commands and their arguments as they are Roland> executed. Roland> -- snip -- Roland> The basic idea is to see which exact command failed when a Roland> problem occurs. Okay, I thought it might be something like that. I'm concerned about extra noise in the build and having to add something to nightly.sh to filter it out. But I'm okay with leaving it in for now and seeing if it's really a problem or just my paranoia. ;-) >> * usr/src/cmd/ksh/Makefile >> - where are we with i18n? will we have a real ksh93.po prior to >> putback? Roland> I think you're asking here about l10n (=localisation), right ? Well, i18n and l10n both. I hadn't realized that the i18n issues had been addressed, so I assumed that the l10n stuff was turned off because we were waiting for the i18n fixes. Roland> Localisation (AFAIK the *.po stuff is for l10n) is working in Roland> the ksh93 binaries - but I simply didn't investigate the details Roland> including: Roland> - Where are the localisation files used for the different Roland> locales stored The way it works is each consolidation delivers a single messages package to the globalization (g10n) group. The g10n group is responsible for the translations. I assume that either the g10n group or Release Engineering creates the l10n files/packages that get installed. The package that ON delivers to g10n is SUNW0on (it's defined in usr/src/pkgdefs, though I don't think it's part of the standard build). Roland> how can we keep track of changes (syncronisation issue) ? I'm not entirely sure how this is managed currently. Entries in the bug database do have a radio button "does this change affect localization yes/no", but I don't know how that information gets back to the g10n group, or if they even use it. This is probably a good question for Ienup or for the i18n-discuss list. Roland> - The libast API is AFAIK different from the normal Solaris libc Roland> API for that (David/Glenn, please correct me if I am wrong Roland> here) so the *.po stuff will likely not be used. I don't have any strong opinions here. As long as you can work out something with the g10n folks, I'm happy. Roland> - Only a microscopic fraction of languages+encodings supported Roland> by Solaris is currently provided by upstream. We really need Roland> more translators... Hopefully Sun can do the translations and pass them back upstream. >> * usr/src/lib/libshell/common/Makefile >> usr/src/lib/libdll/common/Makefile usr/src/lib/libast/common/Makefile >> - I think that having these files in the ON tree will cause >> confusion. I'd like either to remove them or explain in README's >> that this file is not used for ON. (Maybe use >> lib/<library>/common/README.opensolaris?) Roland> Added to my ToDo list (killing them is an option, but the next Roland> diff between ksh93 and Solairs sources then list these files as Roland> "missing" and the person who does the update then has figure out Roland> the "why ?". Hopefully this would be less of a problem once we have everything under Mercurial. Roland> And these Makefiles provide some interesting (well, Roland> not essential) information, too - so I vote to not delete them Okay. Your proposal for the READMEs looks good. >> * usr/src/lib/libshell/common/Makefile.com >> usr/src/lib/libcmd/Makefile.com usr/src/cmd/ksh/Makefile.com >> usr/src/lib/libdll/Makefile.com usr/src/lib/libast/Makefile.com >> - CPPFLAGS: what breaks when you put /usr/include in front of the >> include list? Several other ON components deal with this by >> using >> >> CPPFLAGS = <stuff> $(CPPFLAGS.master) >> >> If that would work here, I think we should do it--it should help >> maintainability of the ON code as a whole. Roland> Using $(CPPFLAGS.master) is tricky, too - it MUST come as the Roland> last element but future changes in -D may then cause silent Roland> breakage in the AST sources because the last -D specified Roland> overrides previous -D options so I prefer the current way to Roland> expliclity list each single flag. [...] Roland> What should I do here now - leave it as it implemented currently Roland> or try to "squish" $(CPPFLAGS.master) somewhere into the chain Roland> in the hope that this won't bite back in the future... ? I guess I'd leave it as-is. It would be good to have a comment explaining why we're not using $(CPPFLAGS.master) (basically giving the reasoning that you gave). >> I see that the workaround hack for the PAGESIZE redefinition is still >> in place. This will need to be changed prior to putback. Roland> Yes... the problem here is that the PAGESIZE stuff sits in the Roland> automatically generated AST headers (there are two other Roland> locations in the source which have a similar problem and can Roland> easily be "silenced" using the same solution (however until now Roland> I didn't apply _ANY_ changes to the original AST Roland> sources)). Adding a workaround (e.g. |#undef PAGESIZE|) is Roland> possible but the next update of these headers will kill it - and Roland> then we have a build failure which needs to be investigated, Roland> e.g. making this a pain regardless of which way we choose. I'd much rather have a visible build failure than a silent failure due to an undetected macro redefinition. >> * usr/src/lib/libshell/Makefile >> usr/src/lib/libdll/Makefile usr/src/lib/libast/Makefile >> - these still have the comment "64bit disabled for now due lack of >> AMD64 build machine". I thought the issue was that the 64-bit >> support is broken, and that it's being fixed upstream. Roland> Originally I disabled the 64bit support until I had time to Roland> seize a AMD64 box to work on that problem. During that work I Roland> hit the problem that the public includes of libast and libshell Roland> differ between 32bit and 64bit builds. The same problem exists Roland> on SPARC64, too. There are two solutions: Roland> 1. Ship two sets of includes, one for 32bit and one for 64bit Roland> (for example stored at /usr/include/64/), however I am not Roland> sure whether this is worth the trouble [...] Roland> 2. Wait until AST upstream fixed the problem Roland> I like to wait for [2] (and commited a comment change as Roland> http://polaris.blastwave.org/changeset/296) unless you or April Roland> think it's neccesary to do [1] ... I'm fine with waiting for upstream to fix the problem. But if it's not fixed prior to putback, I'd really like to see the 64-bit (non-)support done as cleanly as possible (e.g., don't install any 64-bit symlinks). The new comments (changeset/296) look okay to me. >> * usr/src/lib/Makefile >> - you shouldn't need the new .WAIT's (for libast et al), since you >> have the explicit dependencies listed further down in the makefile Roland> Fixed (see http://polaris.blastwave.org/changeset/295). Looks good. >> * usr/src/lib/libdll/sparc/Makefile >> usr/src/lib/libdll/i386/Makefile usr/src/lib/libast/sparc/Makefile >> usr/src/lib/libast/i386/Makefile >> - HOSTTYPE still has the OS release number hardwired in. If this >> won't actually cause problems for future releases (when uname -r >> moves to 5.12), then we can treat this as a cleanup nit, I think. Roland> It won't cause problems directly but I added that to my ToDo Roland> list to get it fixed somehow prior to the first putback or Roland> shortly after that point... Sounds good. >> * usr/src/lib/libast/i386/Makefile >> usr/src/lib/libast/sparc/Makefile usr/src/lib/libast/Makefile.com >> - these all have code to enable PIC; does that really need to be >> in all 3 places? Roland> AFAIK no, I hope the "fix" in Roland> http://polaris.blastwave.org/changeset/297 is the correct Roland> solution... I believe it's correct. By the way, thanks for the fine-grain checkins and the pointers to the changeset web pages. That makes it much easier to review specific changes. mike
