Just so that I'm clear on this, this webrev: http://cr.opensolaris.org/~error404/6733971-2/ has your go-ahead?
James Carlson wrote: > Roland Mainz writes: > >> James Carlson wrote: >> >>> As for the origin, I went back to the SCCS 1.1 version of >>> $SRC/uts/common/Makefile.rules (from 1991), and the puzzling extra >>> parenthesis are there. That seems to be the oldest version. It's not >>> in SysV or old SunOS. >>> >> Who wrote that file originally ? >> > > The file was checked into SCCS by Joe Kowalski, but that doesn't > actually tell us anything, because that's when it was moved over from > the old NSE, and I don't have access to history prior to that point. > > >>> I refer to the question above: what good do the subshells do? >>> >> We don't have a clue. >> > > That is *precisely* the problem. > > I believe that preserving bizarre bits of implementation voodoo > because nobody understands them is the wrong thing to do. The most > important thing source code has to do is to be understandable -- if it > isn't at least that, then it cannot be maintained, and it doesn't > really matter whether it has or will ever work. > > (For what it's worth, this is exactly why I and many others push for > comments when bits of code are unobvious. Someone years from now is > going to have to figure this thing out again, and knowing whether > something odd was done intentionally ['clever'] or just by accident is > unfortunately often a key bit of information.) > > >> The worst theory we have is "... another [ancient >> and never removed] workaround for bugs in /usr/ccs/bin/make ..." ([1]) >> and the best is "... human error ...". >> >> [1]=Since some make versions have builtin shell expansion functionality >> to avoid extra calls to /usr/bin/sh - but noone knows whether this >> applies here. >> > > Interesting theory, but since (a) it doesn't seem to apply here in any > tests I've done, (b) we have complete control over the build > environment [i.e., makefile level portability is unimportant], and (c) > if there were such bugs in 'make', we'd just fix 'em, I argue that > even if it were an ancient work-around, it needs to be removed. > > >>> If [1] means "don't integrate, but forward along to someone else who >>> can look at it more deeply," then I'd reluctantly go along. I think >>> it's fairly obvious that the subshells do nothing here, but if you (or >>> the actual submitter?) aren't comfortable with that, then doing >>> nothing is a bit safer. >>> >> I disagree. Maybe the subshells have a special function we don't know >> about. Since several of my emails with questions were not answered >> during the time I wrote the previous email my best estimation was to try >> to be as carefull as possible but still trying to get the bug fixed. >> > > That's the "cargo cult" problem I referred to before. > > We can't just make changes without understanding what the system is > doing. If there are parts that don't make sense, aren't documented > such that anyone can understand them, and don't appear to serve any > function, then I believe we're better off ripping them out than > pretending that there's some associated magical behavior. There > shouldn't be undocumented magic here, because magic can't be > maintained. > > >>> If [1] means "integrate as-is now, but then think about it a bit, and >>> maybe rewhack these things again in the future," then I'm opposed to >>> the plan. >>> >> Why ? There are other "janitor" tasks in OS/Net which require fixes >> which need to be done in multiple stages. >> > > When changing many files across the system, especially ones like these > makefiles where cut-n-paste is rampant, it helps a great deal to make > sure that you change them as rarely as possible, particularly when it > comes to style changes. > > There are lots of projects all running at once across ON, and the > chance that they've already copied these existing bad practices is > high, as is the chance that they'll fail to do the merge properly. As > a result, this integration will likely need at least a Heads Up > message for gatelings, so that they know that it's not a run-of-the- > mill merge job. > > Creating multiple such merge points just makes the problem worse, and > makes failure more certain. > > And, more importantly, I see no technical reason at all why this > particular task ought to be done piecemeal. > > >>> I'd be opposed to [2] for essentially the same reason. It seems like >>> a half-baked response to a review comment. >>> >> No, it wasn't intended to be half-baked. It was the attempt to split >> this work into one "normal stuff" and "weired stuff" part and defer the >> "weird stuff" part to the point where we re-work the Makefiles for >> non-POSIX constructs (which eat-up some performance) which requires lots >> of in-depth testing. >> > > I don't think I understand the "non-POSIX constructs" (either what > they are or why they necessarily cost any significant performance in > the build), but I don't believe that the extra subshell level removal > has anything notably to do with that re-work, and it has *everything* > to do with this '(( ))' ksh93 incompatibility problem. > > >>> If [3] means "make a new patch without the extra parenthesis *and* >>> integrate into ON," then I'm in agreement with that. If it means >>> "make a new patch and then just stop," then I don't understand what >>> the point is. >>> >> I've attached a new unified diff (as >> "cr6733971fix_20090107_003.diff.txt.bz2") which just removes the extra >> bracket pair... >> ... is that Ok for you ? >> > > Yes. > >
