At Wednesday 21 July 2010, Ralf Wildenhues wrote: > > > Your changes seem to have valued efficiency so far. But > > > calling perl is bound to be more expensive than a couple of > > > greps. > > > > Yes, but it's blazingly fast if compared with an aclocal/automake > > call. > > Sure, but you're not replacing an aclocal/automake call. If those > are slow, then the solution is to make them faster, not to make > the rest slower. ;-) ... but I think you got what I was truly meaning, didn't you? ;-)
> > > The change looks good to me, how come you went this way though? > > > Just curious. > > > > Because writing that perl script was waaay simpler than cooking > > up a sed script or a sed/grep pipeline doing the same thing > > portably. And even if that perl script isn't much efficient, it > > is very simple to understand IMO. > > You may not agree but I find the perl script way harder to > understand. Really? I'd never thought that possible, at least not for someone which knows perl quite well (like you do, I'd say :-) > And it's not more correct in that it won't survive > line wraps in the Makefile either. Oh, but that was deliberate. After all, if it's quite unpredictable wheter a line is going to be wrapped, it's also unpredictable how a line is going to wrapped, isn't it? So, why bother where backslashes and newlines are placed? > > Maybe you can think of a *simple* way to do that which doesn't > > involve perl? Just curious (no pun intended :-) > > A shell function in defs.in 'extract_makefile_deps TARGET FILE' > that contains a sed script similar to extract_makefile_var that > does what you want, and respects backslash-newline? > > Something like (mind the embedded TABs): > > sed -n ' > /^[ ]*TARGET[ ]*:/b doit > /^[^:]\{1,\}[ ]TARGET[ ]*:/b doit > /^[ ]*TARGET[ ][^:]*:/b doit > /^[^:]\{1,\}[ ]TARGET[ ][^:]*:/b doit > d > > :doit > > /\\$/{ > N > s/\\\n/ / > b doit > } > s/[^:]*://p' > I don't want to seem overly argumentative, but... do you really see this as simple? WOW! (BTW, are `\n' and \{...\} truly portable in sed? I'd have said "no" until a moment ago, but now I think I was most probably mistaken, since you are using them here). > I find this as easy (or as hard) to read as your proposed perl > script, and it is more correct, too. Why is it more correct? And to summarize: are you going to keep the perl script anyway, are you going to amend the patch yourself, or should I amend it (and, in this last case, how)? > > > > --- a/tests/colon5.test > > > > +++ b/tests/colon5.test > > > > [CUT] > > > > > > > > +cat > Makefile.am <<'END' > > > > +.PHONY: test > > > > +test: > > > > + case ' $(DIST_COMMON) ' in \ > > > > + *' $(srcdir)/Makefile.dep '*) exit 0;; \ > > > > > > Hmm. The variable might just as validly list Makefile.dep, > > > without preceding `$(srcdir)/'. That is really necessary only > > > if the file is listed both as a prerequisite and as a target > > > somewhere in the makefile. > > > > But why should we relax this test if it's working correctly? > > After all, Automake is expected to have full control in the > > generation of the autotools-related rebuild rules, right? And > > we can relax the test in the future if the necessity arises. > > Well, this is a common dilemma in testing: should the tester be > allowed to use insider information or not? Sometimes the answer > is yes: we may really want to make sure something is not only the > way the documentation states, but the way it was implemented under > the hood. > > But other times that is not so clear: say, three years down the > road, somebody wants to change something in automake.in, do a > factorization or so. This causes a test failure, due to a test > like colon3 peeking at internal details. This typically causes > much head scratching: was the test bad before, or did we just > introduce a bug with the patch? Yes, this is a valid concern. > Getting the right balance here is the most non-automatic part of > testsuite work. Ideally, we want the testsuite to be as strict as > possible without ever peeking in details which do not matter for > the documented API semantics. So that any change we make to > automake.in produces no testsuite failure if the change still > provides the right user API. Agreed. This would be more feasible and natural if we had unit tests for automake internals... but, alas, ATM I don't see any way to introduce such tests without an *extensive* reorganization and refactoring of Automake code, which would probably (surely?) be way too much dangerous and time-consuming. Oh well. > For a related example of this theoretizing: your various patches > introduce several greps for error message strings. I have acked > them, mostly because I think it is a good idea, generally. Now, a > while ago I started a patch series to add i18n to Automake. Ouch. I really hoped that Automake could have remained i18n-free (yes, I have a strong dislike for i18n, esp. w.r.t. command-line tools: new added complexity to gain so little -- and to lose greppability of error and warning messages in the meantime... sigh). > Each new such grep causes another test failure when the locale is > not English. Of course, there is a simple remedy: set LC_ALL=C in > defs.in, but also of course, that cuts us out of a part of the > code path our users are seeing. Another remedy would be not to introduce i18n in Automake... ;-) > In this particular example, the balance is easy to judge: if the > testsuite covers all error messages Automake can generate, we have > an easy way to automatically ensure all messages really are > annotated for translation if all annotated ones are translated > (which gettext can verify for me). Sorry, I can't understand what you're saying here... > But I am still not sure > whether to localize the grep or override the locale, when it comes > to merge the code. Both have merits. At least in this case a > test failure is easy to analyze. > > For the moment, I haven't changed this fragment. Obviously, you > > are free to amend it yourself if you still think it's too > > strict. > > Yes, the test is too strict that way. Agreed. So, should I amend the test myself? If yes, is this ok? case ' $(DIST_COMMON) ' in *' $(srcdir)/Makefile.dep '*|*' Makefile.dep '*) exit 0;; ... Or should I be even more lax, and do this? case ' $(DIST_COMMON) ' in *' Makefile.dep '*|*'/Makefile.dep '*) exit 0;; ... Regards, Stefano