Bruno Haible wrote: >> I'll wait for feedback before pushing. > > OK for me in general. However, I would combine some Makefile commands > before adding the $(AM_V_GEN) placeholder. When you put $(AM_V_GEN) > in front of the 'rm -f $...@-t $@' commands but use '$(AM_V_at)' in front of > the command that actually creates the file, it's likely to be broken > in the future. Two years from now, few developers will remember or know > what $(AM_V_GEN) and $(AM_V_at) actually mean and how to place them (*), > since it's not a particularly important aspect of automake. I can easily > see people changing > > $(AM_V_GEN)rm -f $...@-t $@ > > into > > $(AM_V_GEN)rm -f $...@-t > $(AM_V_GEN)rm -f $@
I agree that the issue is subtle and easy to overlook, but I'm confident that none of the regular contributors here would make that particular change. However, minimizing uses of AM_V_at seems like an improvement, so I pushed your modified patch as-is. > - leading to duplicated GEN lines - or removing the $(AM_V_GEN) > entirely, seeing that the majority of the lines uses $(AM_V_at), - > leading to no output at all -. > > (*) In fact, automake does not even document the important rule how to > distribute $(AM_V_GEN) and $(AM_V_at). The rule is: "In every target, > you should use $(AM_V_GEN) exactly once and $(AM_V_at) for all other > commands in the Makefile target." Maintainers have to gain experience > with silent-rules in order to guess this rule! > > So I would propose to combine the Makefile commands and minimize the use > of $(AM_V_at): ... > Here is the complete patch. This patch also handles the 'localcharset' module. However, I will follow up with a change to join the continued lines like this, - } > $...@-t && \ - mv $...@-t $@ + } > $...@-t && mv $...@-t $@ so that the conceptual redirect-to-$@ is performed all on one line. That way, it's less likely that someone will accidentally insert something between the redirect and the "mv".
