automake-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 04/10] Refactoring: new $automake_remake_options global varia


From: Ralf Wildenhues
Subject: Re: [PATCH 04/10] Refactoring: new $automake_remake_options global variable.
Date: Mon, 3 Jan 2011 22:48:55 +0100
User-agent: Mutt/1.5.20 (2010-08-04)

Hello Stefano,

* Stefano Lattarini wrote on Sun, Jan 02, 2011 at 11:57:42PM CET:
> On Sunday 02 January 2011, Ralf Wildenhues wrote:
> > * address@hidden wrote on Thu, Dec 23, 2010 at 12:27:40PM CET:
> > > This change is useful only in view of soon-to-follow refactorings
> > > and simplifications, related to the fixing of Automake bug#7669
> > > a.k.a. PR/547.

> > This is the first patch I'm having high-level concerns with.  Basically,
> > I don't understand why patches 04, 05, 06, and 09 are needed at all for
> > fixing the particular issue this patch series aims to fix.
> >
> Well, while they might not be *strictly* needed, they removed some
> unnecessary complexity and *greatly* helped me realize what steps I
> had to take in order to fix the bug in question [1].  These steps
> might become more complicated or numerous without the changes in
> the patches you're referring to.
> 
> [1] When a refactoring helps in making a bugfix (almost) obvious,
>     I think it has very good chances of being a good refactoring.

That may well be true in general.  However, the refactoring you propose
has two distinct downsides which are not good:

- it moves away from encapsulated data (Options.pm) toward global
  variables,
- it enshrines behavior that we know (or at least strongly suspect) to
  be broken.

Please do not try to hold your fixes hostage of such a questionable
refactoring.  Please take a step back from your work done, and do not
try to cling to your code.

I just saw http://tirania.org/blog/archive/2010/Dec-31.html and while I
don't think it is completely fitting in this case (as you are not all
new to Automake, nor is this refactoring completely trivial), I still
think that such a step warrants a good reason and discussion
independently of the bug that you are fixing.

So no, without an argument why the refactoring is necessary in order to
fix the bug (rather than just understand it), I'm sorry but I have to
reject it, on the grounds that it causes a net code quality regression.
Even if it turns out to be necessary, I think I would ask you to
reformulate it in terms that avoid the above downsides.

I'm not sure if it obvious, but I often try out code, or change code,
when working on a bug fix; and then in the end, I see that a lot of the
changes turn out to not be necessary to actually fix the bug.  The right
strategy then is to go back and make a smaller, or even minimal, fix.
The final patch may look a lot different from the way to get there.
Generally small fixes also leads to few regressions, as usually the
number of regressions is proportional to the amount of code changed.

> > I also don't think that moving from module-local variables and
> > concepts (options vs. global_options)
> >
> That was maybe a good distinction when it was first introduced; but
> that was before automake started to trace autoconf macros and to allow
> options to be specified in AM_INIT_AUTOMAKE.  Now the distinction has
> bitrotten somewhat -- if we really still want a meaningful distinction,
> we should aim at a threefold one:
>   (cmdline options) vs. (global options) vs. (local options)

Ah.  So the refactoring should at most be part of fixing that other bug.

> But such a more granular distinction is not needed ATM, so I've preferred
> to go by the shortest route.  Once the bugfix and the tests are in place,
> we could go for a second round of refactorings.

But we don't ever need to do the first one!

> Additionaly, you can easily see that the pre-existing distinction between
> global and local automake options was *really* needed *only* in the
> `handle_configure' subroutine, so the complexity it entailed was IMHO
> not really justified (at least w.r.t. the current form of the codebase).
> 
> > toward global variables like am_remake_options would be
> > an improvement.
> >
> In the long run, that variable should disappear too;

No, that global variable should never exist IMHO.

> i.e., either
> we are going not to meddle with command-line options outside of
> the parse_arguments subroutine [1], or we are going to introduce
> the threefold distinction I cited above.
> 
> [1] This might be the case if we'll decide that automake-generated
>     rebuild rules should not propagate command-line options derived
>     either from the AM_INIT_AUTOMAKE call or from the first automake
>     invocation.  But that's material for another thread (I have an
>     unfinished patch about this in my local repository, but it depends
>     on the current patch series being applied first).

> > On the contrary, so far I've thought that the
> > distinction and current API would be a good one, no?  What makes it
> > unsuitable?
> >
> I hope you'll find the above argumentations and explanations convincing
> enough as an answer to this question.

Well, you don't answer the question why the current API is not suitable.

> > Why are you even changing code that deals with the rerun arguments if
> > you are not fixing the issues with it?  This is not a rhetorical
> > question, I might again just be overlooking all the keys.
> >
> Because I need some refactorings to make my fix look "simple & natural",
> but those refactorings might have an impact on the external behaviour of
> the automake-generated rebuild rules if those other modifications you're
> referring to are not applied first.
> 
> To put it in another way,  we are trading some old complexity with some
> new complications in order to keep the automake behaviour as unchanged
> as possible in this patch series; in the long run, we should remove this
> additional complication by either changing the semantics of the automake
> rebuild rules (if that's deemed correct), or by introducing a more
> proper complexity (with less "historical burdens" attached to it).

Let's then go for simplicity and simplify the rebuilding rules.

> > I think it's great that you have
> > attacked this bug and written this patch series!  I just want to make
> > sure we don't regress in code quality while fixing user-visible bugs.
> >
> I honestly do believe that this patch series might be a useful first
> step in a journey to tidy up the automake codebase.  I hope not to
> be fooling o deluding myself on this!

Again, bug fixing and tidying up are different tasks.  I am generally
very suspicious of the latter when the advantages aren't clear (and even
when there aren't any disadvantages on the road, unlike here).

> > > -  my $automake_options = '--' . (global_option 'cygnus' ? 'cygnus' : 
> > > $strictness_name)
> > > -                  . (global_option 'no-dependencies' ? ' --ignore-deps' 
> > > : '');
> > > +  my $automake_options = '--' . $am_remake_options{strictness}
> > > +                       . ($am_remake_options{ignore_deps} ?
> > > +                          ' --ignore-deps' : '');
> > 
> > Wow.  I didn't know it was perl-strict-clean to put barewords inside
> > hash derefs a la $hash{word}.
> >
> Yes, for good or for bad, it is. Unfortunately, I cannot remember if
> there's an easy way to forbid the use of barewords as hash keys
> (google is not much helpulf on the issue).
> 
> > That seems like an undesirable feature
> > to me, because it is easy to forget a $ as in $hash{$word}.
> > Can we please not make use of this, it looks like a typo above already
> > (with $strictness being a valid variable name)?  Thanks.
> >
> Fine with me; I amended the patch to do so.

Thank you, good we agree on this.

> > > --- /dev/null
> > > +++ b/tests/remake-am-opts.test

> > > +# Test that remake rules calls automake with proper command-line
> > > +# options derived from options in AM_INIT_AUTOMAKE, or passed on
> > > +# the command line of the original invocation.
> > > +#
> > > +# NOTE: That just described above might not be a very smart semantic,

> > > +# after all.  See this message related to automake bug #7673:
> > > +#  <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7673#11>
> > > +# Update this testcase if the semantic is changed.

> > It seems like this test would be testing functionality that is dubious
> > at best and wrong at worst.  Why are we doing this?
> >
> To ensure that the later patches doesn't change this functionality
> behind our backs. If we break backward compatibility, even for dubious
> features, we should do so consciously and deliberately, and not through
> unexpected side effects of fundamentally unrelated changes.

Agreed.  But maybe we should just break this functionality consciously,
as we have deemed it buggy?  Anyway, I don't think we should add
testsuite coverage for suspected-wrong functionality.  That's just
wrong, and expending work on the wrong bits.

[...]
> > Don't you think this is a bit more complex than necessary?
> >
> Maybe, but I preferred to err on the side of caution.
> 
> Also, for what concerns maintainability, my "secret" hope is that, in
> a not-so-distant future, we could change the semantics of the rebuild
> rules, and then get rid of this testcase (or rewrite it to check the
> opposite behaviour, which would make it definitely simpler).

Well, change the rebuild rule semantics now (or in a prior patch series)
if that helps.  I don't think it is necessary to do either way though.

> > What is this supposed to do, and why?
> >
> Check that the automake instances invoked by the automake-generated
> rebuild rules are passed the expected options (and only those options).
> 
> > Why is the same not achieved with a couple of greps in the usage
> > part of the test below?
> >
> Because the rebuild rules are run "silenced" (their recipe is
> prepended with a `@'), and use calls to "echo" to show what commands
> they're about to perform [1].  These displayed commands might thus
> not correspond to the real ones, and we want to check both then.

Wow.  Your mistrust is honorable, but it might go beyond what I would
expect from a test suite.

Cheers,
Ralf



reply via email to

[Prev in Thread] Current Thread [Next in Thread]