[illumos-Developer] Review 693

[Igor Kozhukhov]

Hi Richard,

Thanks for your comments - will fix.
I was interested in integration comments.

Bets regards,
Igor


> From: Richard Lowe <richlowe at richlowe.net>
> Date: Wed, 13 Jul 2011 11:18:37 -0400
> To: Igor Kozhukhov <ikozhukhov at gmail.com>
> Cc: Alexander Eremin <alexander.eremin at nexenta.com>,
> "developer at lists.illumos.org" <developer at lists.illumos.org>
> Subject: Re: [illumos-Developer] Review 693
> 
>> At this moment it is first integration and we can change what we need later
>> - maybe update to new versions, maybe restructure if needed.
>> 
> 
> No, we need to get it right the first time.  90% of the really
> terrible parts of our source tree are from the integration of formally
> separate projects to be "fixed up later", it almost always goes
> horribly wrong (look at usr/src/man and the lp sources, if you want
> examples)
> 
> - I'm generally concerned by the need to add -I$(ROOT)/usr/include and
> variations to Makefiles.  Were all these components previously broken,
> and not using the proto includes?  In general, if something does
> "CPPFLAGS =" the bug is that they're not appending to CPPFLAGS, rather
> than anything specific about the contents.
> 
>   Using just $(ROOT)/... means that these components will ignore
> ENVCPPFLAGS even after you're done.
> 
>   In a couple of cases you seem to be adding $(ROOT)/usr/include even
> though CPPFLAGS _is_ being appended to.  What am I missing?
> 
> - Remove the SCCS keywords throughout (the #pragma ident, #ident, etc, lines)
> 
> usr/src/lib/Makefile:
> 
>   I'd far prefer that rather than using .WAIT after libm, that you
> call out the dependencies explicitly (I'm assuming there are few
> enough of them that this is worthwhile, if there aren't, do let me
> know).  The use of .WAIT in there is almost entirely historical and
> buggy (libc is a special case, obviously).
> 
> usr/src/lib/libm/amd64/Makefile:
> 
>  The presence of any mention of sparc in here is deeply suspicious.
> Should a bunch of this be deleted?  In a Makefile.com (it certainly
> looks like a bunch of it should be in a Makefile.com)
> 
>   34:
>     Don't use SUNW_ISA (and an :sh) rule, just use $(MACH64) or
> $(MACH) as appropriate.
> 
>   40,42:  Remove commented code
> 
>   70,75: All of this should be removed, and Makefile.master allowed to
> do the same thing.
> 
>   138: Almost certainly shouldn't be there.
> 
>   145: I can't immediately tell what the libm folks were doing there,
> but I'd vastly prefer that they didn't.
> 
>   154,155: Remove commented code
> 
>   169: Should be available from the make structure described in lib/README*
> 
>   441: Remove commented objects
> 
>   925: Remove commented rule
> 
>   943,1033: More libm crazyness.  Do we actually make use of these rules?
> 
> usr/src/lib/libm/amd64/mapfiles/*-sparc, v9a, v9b, etc.
> 
>   Why are there sparc mapfiles in this amd64 directory?
> 
> usr/src/lib/libm/i386/Integration.log:  Delete
> 
> usr/src/lib/libm/i386/Makefile:
> 
>   A bunch of (most) of this is duplicated with amd64/Makefile.  Put
> the common pieces in usr/src/lib/libm/Makefile.com and include them.
> (otherwise, all my comments from the amd64 Makefile apply here too).
> 
> usr/src/lib/libm/i386/llib-lm:  This looks the same as the amd64 copy,
> if it is, it should be common, not duplicated.
> 
> usr/src/lib/libm/i386/mapfiles/*:  Same comments as for amd64, but if
> these are all duplicated, make them common, don't have two copies.
> 
> I can't tell if the weird libm source organization is too well baked
> in to change to be conventional (common sources in common/ platform
> specifics in the i386/ and sparc/ top-level directories, etc.), if it
> turns out after fixing the Makefile comments above that this is easy,
> it'd be great if you did it.
> 
> I haven' looked at any of the libm sources, just the build
> infrastructure.  I'll read through your diffs as compared to the libm
> tarball in a while (or, hopefully, someone else will beat me to it)
> 
> Thanks for doing this,
> 
> -- Rich