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. -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
