James Carlson wrote: > Roland Mainz writes: > > James Carlson wrote: > > > John Sonnenschein writes: > > > > code at: > > > > http://cr.opensolaris.org/~error404/6733971/ > > > > > > Instead of introducing spaces -- which I think makes the code just a > > > little more obscure -- why not get rid of the superfluous parenthesis? > > > > Good question... the answer is that we do not know exactly _why_ this > > stuff was originally added with subshells. It seems to be done > > intentionally but noone knows _why_. > > At least for me, that's just not a good enough answer. There > shouldn't be any mystery here, particularly for a change that's going > to be made in a whole pile of files at once and that is small enough > to have reasonably well-understood results. > > 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 ? > Given that, and the fact that the code does exactly the same thing > with or without the redundant parenthesis, I'm claiming that it's just > an ancient mistake and shouldn't be embellished. See for the various theories below... but after all I've attached a patch which just removes the extra brackets (see comment at the bottom of this email). > > > That way, there's no question for future maintainers, and both forms > > > appear to be equivalent for ksh93. > > > > > > Are all these subshells doing us any good? > > > > As said above I tried to get an answer and so far the investigation was > > for /dev/null. My gut feeling says we should keep them. > > Why? > > I refer to the question above: what good do the subshells do? We don't have a clue. 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. > > Options so far are: > > 1. Keep the patch in the current state and forward the investigation to > > the ToDo list of the OS/Net Janitor project > > 2. Keep the patch and ignore the extra brackets (they cause an extra > > |fork()| for the original Bourne shell but ksh93 does not |fork()| for > > subshells which will make this efectively a no-op) > > 3. Make a new patch and remove the extra brackets > > > > I would prefer [1] in this case since [3] would turn this patch IMO into > > a high-risk patch and [2] is IMO just reckless. > > I don't understand [1]. I can't tell which of those options (if any) > actually leads to integrating something into the ON gate, and, if so, > what gets integrated. I don't know what "keep the patch" means in > this context, because I don't know where you're saying it should be > kept. > > 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. > 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. > They should be fixed, and fixed just once. Don't force > everyone else through needless merges. Right... > 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. > 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 ? ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;) -------------- next part -------------- A non-text attachment was scrubbed... Name: cr6733971fix_20090107_003.diff.txt.bz2 Type: application/octet-stream Size: 8730 bytes Desc: not available URL: <http://mail.opensolaris.org/pipermail/on-discuss/attachments/20090111/a345f140/attachment.obj>
