[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RFC: diagnose target clashes (for PR/344)
|
From: |
Alexandre Duret-Lutz |
|
Subject: |
RFC: diagnose target clashes (for PR/344) |
|
Date: |
11 Sep 2002 00:57:38 +0200 |
|
User-agent: |
Gnus/5.0808 (Gnus v5.8.8) Emacs/20.7 |
I'd really appreciate comments on the following patch.
The intent is to diagnose target clashes.
For instance if the user define `bin_PROGRAMS = ctags' Automake
must detect that this conflict with the `ctags:' target (a new
target introduced in Automake 1.7 to create `CTAGS' files using
`ctags').
This is the case reported in PR/344: specflag7.test and
specflag8.test are tests for the ctags/etags example of the
manual, where a `ctags' program is built. However, since the
addition of the `ctags:' target, these two tests have started
running `ctags' instead of building `ctags' :)
With this patch, these two test cases fails with something like
[...]/lib/am/tags.am: redefinition of `ctags'...
[...]/lib/am/program.am: ... `ctags' previously defined here.
The locations could be improved but that's not the point here;
for now I'm just using whatever location is available. At least
Automake doesn't produce a bogus Makefile silently.
Basically, the idea is to detect clashes in &rule_define, which
is called every time a new rule is defined. However it turned
out to be harder than I thought:
1. &file_contents_internal used to contain a hack to define
rules only for the set of conditions where they haven't
been defined already. So &rule_define was not called when
the rule was already defined (making clash detection from
&rule_define somewhat hard...). I have moved this hack
into &rule_define.
Richard, since you wrote this code it would be nice if you
could review the rational I've added in a comment, in case
I missed the real intent.
2. &conditional_ambiguous_p used to work only for variables,
but was used on targets by &rule_define. I've generatilzed
&conditional_ambiguous_p to fix this.
3. The purpose of %target_source is to record the file where
a target comes from. I've added this because I didn't want
to diagnose clashes coming from the same file (this concerns
only Automake targets, not user targets): this should allow
us to process the same Makefile fragment several times even
if one of its rules is constant. This seems desirable to
me, although I'm not sure it's needed currently.
I've not run the full test suite yet (only the first half of it
plus specflag{7,8}.test); it's too late now.
As far PR/344 is concerned, what's needed after this patch is
1. update specflag7.test and specflag8.test to reflect they
now test for this feature
2. update ctags/etags example in the manual (probably change
it to egrep/fgrep...)
3. add tests cases for the new manual example (since
specflag7.test and specflag8.test no longer test this)
4. implement the _NAME thing or any other proposal to make
the ctags/etags build possible.
I think #4 can be kept for after 1.7 is released.
2002-09-11 Alexandre Duret-Lutz <address@hidden>
Diagnose target clashes, for PR automake/344:
* automake.in (%targets): Records conditions for definitions.
(%target_conditional): Remove (obsoleted by %targets).
(%target_source, %target_owner): New hashes.
(TARGET_AUTOMAKE, TARGET_USER): New constants.
(initialize_per_input): Adjust to reset new variables.
(err_cond_target, msg_cond_target): New functions.
(msg_target): Adjust usage of %targets.
(conditional_ambiguous_p): Take a list of condition to check
as a third parameter, so this can be used for other things that
variables.
(handle_lib_objects_cond): Adjust conditional_ambiguous_p usage.
(variable_defined): Use reject_target.
(rule_define): Add the $SOURCE argument, and take $OWNER instead
of $IS_AM. Diagnose rules clashes (including ambugious conditions).
Return a list of conditions where the rule should be defined instead
of a boolean. Fill %target_source and %target_owner.
(target_define): Use `exists', not `defined'.
(read_am_file): Adjust the call to rule_define.
(file_contents_internal): Add more FIXMEs. Simplify my moving
and documenting the "define rules in undefined conditions" to
rule_define.
* tests/Makefile.am (XFAIL_TESTS): Add specflags7.test and
specflags8.test.
Index: automake.in
===================================================================
RCS file: /cvs/automake/automake/automake.in,v
retrieving revision 1.1347
diff -u -r1.1347 automake.in
--- automake.in 10 Sep 2002 20:45:57 -0000 1.1347
+++ automake.in 10 Sep 2002 22:32:36 -0000
@@ -517,11 +517,19 @@
my %content_seen;
# This holds the names which are targets. These also appear in
-# %contents.
+# %contents. $targets{TARGET}{COND} is the location of the definition
+# of TARGET for condition COND.
my %targets;
-# Same as %VAR_VALUE, but for targets.
-my %target_conditional;
+# $target_source{TARGET}{COND} is the filename where TARGET
+# were defined for condition COND. Note this must be a
+# filename, *without* any line number.
+my %target_source;
+
+# $target_owner{TARGET}{COND} the owner of TARGET in condition COND.
+my %target_owner;
+use constant TARGET_AUTOMAKE => 0; # Target defined by Automake.
+use constant TARGET_USER => 1; # Target defined in the user's Makefile.am.
# This is the conditional stack.
my @cond_stack;
@@ -721,8 +729,8 @@
%content_seen = ();
%targets = ();
-
- %target_conditional = ();
+ %target_source = ();
+ %target_owner = ();
@cond_stack = ();
@@ -1186,6 +1194,14 @@
msg_target ('error', @_);
}
+# err_cond_target ($COND, $TARGETNAME, $MESSAGE, [%OPTIONS])
+# ----------------------------------------------------------
+# Uncategorized errors about conditional targets.
+sub err_cond_target ($$$;%)
+{
+ msg_cond_target ('error', @_);
+}
+
# err_am ($MESSAGE, [%OPTIONS])
# -----------------------------
# Uncategorized errors about the current Makefile.am.
@@ -1211,13 +1227,24 @@
msg $channel, $var_location{$macro}, $msg, %opts;
}
+# msg_cond_target ($CHANNEL, $COND, $TARGETNAME, $MESSAGE, [%OPTIONS])
+# --------------------------------------------------------------------
+# Messages about conditional targets.
+sub msg_cond_target ($$$$;%)
+{
+ my ($channel, $cond, $target, $msg, %opts) = @_;
+ msg $channel, $targets{$target}{$cond}, $msg, %opts;
+}
+
# msg_target ($CHANNEL, $TARGETNAME, $MESSAGE, [%OPTIONS])
# --------------------------------------------------------
# Messages about targets.
sub msg_target ($$$;%)
{
my ($channel, $target, $msg, %opts) = @_;
- msg $channel, $targets{$target}, $msg, %opts;
+ # Don't know which condition to use. Pick any.
+ my $cond = (keys %targets)[0];
+ msg_cond_target ($channel, $cond, $target, $msg, %opts);
}
# msg_am ($CHANNEL, $MESSAGE, [%OPTIONS])
@@ -2913,21 +2940,22 @@
}
}
- if ($xname ne '')
+ if ($xname ne '')
{
- if (conditional_ambiguous_p ($xname . '_DEPENDENCIES', $cond) ne '')
+ my $depvar = $xname . '_DEPENDENCIES';
+ if ((conditional_ambiguous_p ($depvar, $cond,
+ keys %{$var_value{$depvar}}))[0] ne '')
{
- # Note that we've examined this.
- &examine_variable ($xname . '_DEPENDENCIES');
+ # Note that we've examined this.
+ &examine_variable ($depvar);
}
- else
+ else
{
- define_pretty_variable ($xname . '_DEPENDENCIES', $cond,
- @dep_list);
+ define_pretty_variable ($depvar, $cond, @dep_list);
}
}
- return $seen_libobjs;
+ return $seen_libobjs;
}
# Canonicalize the input parameter
@@ -6034,51 +6062,54 @@
sub check_ambiguous_conditional ($$)
{
my ($var, $cond) = @_;
- my $message = conditional_ambiguous_p ($var, $cond);
+ my ($message, $ambig_cond) =
+ conditional_ambiguous_p ($var, $cond, keys %{$var_value{$var}});
err_var $var, "$message\n" . macro_dump ($var)
if $message;
}
-# $STRING
-# conditional_ambiguous_p ($VAR, $COND)
-# -------------------------------------
-# Check for an ambiguous conditional. Return an error message if we
-# have one, the empty string otherwise.
-sub conditional_ambiguous_p ($$)
+# $STRING, $AMBIG_COND
+# conditional_ambiguous_p ($WHAT, $COND, @CONDS)
+# ----------------------------------------------
+# Check for an ambiguous conditional. Return an error message and
+# the other condition involved if we have one, two empty strings otherwise.
+# WHAT: the thing being defined
+# COND: the condition under which is is being defined
+# CONDS: the conditons under which is had already been defined
+sub conditional_ambiguous_p ($$@)
{
- my ($var, $cond) = @_;
- foreach my $vcond (keys %{$var_value{$var}})
+ my ($var, $cond, @conds) = @_;
+ foreach my $vcond (@conds)
{
- # Note that these rules doesn't consider the following
- # example as ambiguous.
- #
- # if COND1
- # FOO = foo
- # endif
- # if COND2
- # FOO = bar
- # endif
- #
- # It's up to the user to not define COND1 and COND2
- # simultaneously.
- my $message;
- if ($vcond eq $cond)
- {
- return "$var multiply defined in condition $cond";
- }
- elsif (&conditional_true_when ($vcond, $cond))
- {
- return ("$var was already defined in condition $vcond, "
- . "which implies condition $cond");
- }
- elsif (&conditional_true_when ($cond, $vcond))
- {
- return ("$var was already defined in condition $vcond, "
- . "which is implied by condition $cond");
- }
- }
-
- return '';
+ # Note that these rules doesn't consider the following
+ # example as ambiguous.
+ #
+ # if COND1
+ # FOO = foo
+ # endif
+ # if COND2
+ # FOO = bar
+ # endif
+ #
+ # It's up to the user to not define COND1 and COND2
+ # simultaneously.
+ my $message;
+ if ($vcond eq $cond)
+ {
+ return ("$var multiply defined in condition $cond", $vcond);
+ }
+ elsif (&conditional_true_when ($vcond, $cond))
+ {
+ return ("$var was already defined in condition $vcond, "
+ . "which implies condition $cond", $vcond);
+ }
+ elsif (&conditional_true_when ($cond, $vcond))
+ {
+ return ("$var was already defined in condition $vcond, "
+ . "which is implied by condition $cond", $vcond);
+ }
+ }
+ return ('', '');
}
# @MISSING_CONDS
@@ -6463,8 +6494,7 @@
# don't want.
if (!exists $var_value{$var})
{
- err_target $var, "`$var' is a target; expected a variable"
- if defined $targets{$var};
+ reject_target $var, "`$var' is a target; expected a variable";
# The variable is not defined
return 0;
}
@@ -7287,22 +7317,27 @@
}
}
-# $BOOL
-# rule_define ($TARGET, $IS_AM, $COND, $WHERE)
-# --------------------------------------------
-# Define a new rule. $TARGET is the rule name. $IS_AM is a boolean
-# which is true if the new rule is defined by the user. $COND is the
-# condition under which the rule is defined. $WHERE is where the rule
-# is defined (file name or line number). Returns true if it is ok to
-# define the rule, false otherwise.
-sub rule_define ($$$$)
+# @CONDS
+# rule_define ($TARGET, $SOURCE, $OWNER, $COND, $WHERE)
+# -----------------------------------------------------
+# Define a new rule. $TARGET is the rule name. $SOURCE
+# si the filename the rule comes from. $OWNER is the
+# owener of the rule (TARGET_AUTOMAKE or TARGET_USER).
+# $COND is the condition string under which the rule is defined.
+# $WHERE is where the rule is defined (file name and/or line number).
+# Returns a (possibly empty) list of conditions where the rule
+# should be defined.
+sub rule_define ($$$$$)
{
- my ($target, $rule_is_am, $cond, $where) = @_;
+ my ($target, $source, $owner, $cond, $where) = @_;
+
+ # Don't even think about defining a rule in condition FALSE.
+ return () if $cond eq 'FALSE';
# For now `foo:' will override `foo$(EXEEXT):'. This is temporary,
# though, so we emit a warning.
(my $noexe = $target) =~ s,\$\(EXEEXT\)$,,;
- if ($noexe ne $target && defined $targets{$noexe})
+ if ($noexe ne $target && exists $targets{$noexe}{$cond})
{
# The no-exeext option enables this feature.
if (! defined $options{'no-exeext'})
@@ -7312,29 +7347,141 @@
. "change your target to read `$noexe\$(EXEEXT)'");
}
# Don't define.
- return 0;
+ return ();
}
- reject_target ($target,
- "$target defined both conditionally and unconditionally")
- if ($cond
- ? ! exists $target_conditional{$target}
- : exists $target_conditional{$target});
+ $target = $noexe;
+
+ # Diagnose target redefinitions.
+ if (exists $target_source{$target}{$cond})
+ {
+ # Sanity checks.
+ prog_error ("\$target_source{$target}{$cond} exists, but \$target_owner"
+ . " doesn't.")
+ unless exists $target_owner{$target}{$cond};
+ prog_error ("\$target_source{$target}{$cond} exists, but \$targets"
+ . " doesn't.")
+ unless exists $targets{$target}{$cond};
+
+ my $oldowner = $target_owner{$target}{$cond};
+
+ # Don't mention true conditions in diagnostics.
+ my $condmsg = $cond ne 'TRUE' ? " in condition `$cond'" : '';
+
+ if ($owner == TARGET_USER)
+ {
+ if ($oldowner eq TARGET_USER)
+ {
+ err ($where, "redefinition of `$target'$condmsg...");
+ err_cond_target ($cond, $target,
+ "... `$target' previously defined here.");
+ return ();
+ }
+ else
+ {
+ # Since we parse the user Makefile.am before reading
+ # the Automake fragments, this condition should never happen.
+ prog_error ("user target `$target' seen after Automake's "
+ . "definition\nfrom `$targets{$target}$condmsg'");
+ }
+ }
+ else # $owner == TARGET_AUTOMAKE
+ {
+ if ($oldowner == TARGET_USER)
+ {
+ # Don't overwrite the user definition of TARGET.
+ return ();
+ }
+ else # $oldowner == TARGET_AUTOMAKE
+ {
+ # Automake should ignore redefinitions of its own
+ # rules if they came from the same file. This makes
+ # it easier to process a Makefile fragment several times.
+ # Hower it's an error if the target is defined in many
+ # files. E.g., the user might be using bin_PROGRAMS = ctags
+ # which clashes with our `ctags' rule.
+ # (It would be more accurate if we had a way to compare
+ # the *content* of both rules. Then $targets_source would
+ # be useless.)
+ my $oldsource = $target_source{$target}{$cond};
+ return () if $source eq $oldsource;
+
+ err ($where, "redefinition of `$target'$condmsg...");
+ err_cond_target ($cond, $target,
+ "... `$target' previously defined here.");
+ return ();
+ }
+ }
+ # Never reached.
+ prog_error ("Unreachable place reached.");
+ }
# A GNU make-style pattern rule has a single "%" in the target name.
msg ('portability', $where,
"`%'-style pattern rules are a GNU make extension")
if $target =~ /^[^%]*%[^%]*$/;
- # Value here doesn't matter; for targets we only note existence.
- $targets{$target} = $where;
- if ($cond)
- {
- if ($target_conditional{$target})
+ # Conditions for which the rule should be defined.
+ my @conds = $cond;
+
+ # Check ambiguous conditional definitions.
+ my ($message, $ambig_cond) =
+ conditional_ambiguous_p ($target, $cond, keys %{$targets{$target}});
+ if ($message) # We have an ambiguty.
+ {
+ if ($owner == TARGET_USER)
+ {
+ # For user rules, just diagnose the ambiguity.
+ err $where, "$message ...";
+ err_cond_target ($ambig_cond, $target,
+ "... `$target' previously defined here.");
+ return ();
+ }
+ else
{
- &check_ambiguous_conditional ($target, $cond);
+ # FIXME: for Automake rules, we can't diagnose ambiguities yet.
+ # The point is that Automake doesn't propagate conditionals
+ # everywhere. For instance &handle_PROGRAMS doesn't care if
+ # bin_PROGRAMS was defined conditionally or not.
+ # On the following input
+ # if COND1
+ # foo:
+ # ...
+ # else
+ # bin_PROGRAMS = foo
+ # endif
+ # &handle_PROGRAMS will attempt to define a `foo:' rule
+ # in condition TRUE (which conflicts with COND1). Fixing
+ # this in &handle_PROGRAMS and siblings seems hard: you'd
+ # have to explain &file_contents what to do with a
+ # conditional. So for now we do our best *here*. If `foo:'
+ # was already defined in condition COND1 and we want to define
+ # it in condition TRUE, then define it only in condition !COND1.
+ # (See cond14.test and cond15.test for some test cases.)
+ my @defined_conds = keys %{$targets{$target}};
+ @conds = ();
+ for my $undefined_cond (invert_conditions(@defined_conds))
+ {
+ push @conds, make_condition ($cond, $undefined_cond);
+ }
+ # No conditions left to define the rule.
+ # Warn, because our workaround is meaningless in this case.
+ if (scalar @conds == 0)
+ {
+ err $where, "$message ...";
+ err_cond_target ($ambig_cond, $target,
+ "... `$target' previously defined here.");
+ return ();
+ }
}
- $target_conditional{$target}{$cond} = $where;
+ }
+
+ # Finally define this rule.
+ for my $c (@conds)
+ {
+ $targets{$target}{$c} = $where;
+ $target_source{$target}{$c} = $source;
+ $target_owner{$target}{$c} = $owner;
}
# Check the rule for being a suffix rule. If so, store in a hash.
@@ -7350,7 +7497,10 @@
register_suffix_rule ($where, $1, $2);
}
- return 1;
+ # Return "" instead of TRUE so it can be used with make_paragraphs
+ # directly.
+ return "" if 1 == @conds && $conds[0] eq 'TRUE';
+ return @conds;
}
@@ -7358,7 +7508,7 @@
sub target_defined
{
my ($target) = @_;
- return defined $targets{$target};
+ return exists $targets{$target};
}
@@ -7538,7 +7688,11 @@
# Found a rule.
$prev_state = IN_RULE_DEF;
- rule_define ($1, 0, $cond, $here);
+ # For TARGET_USER rules, rule_define won't reject a rule
+ # without diagnosic an error. So we go on and ignore the
+ # return value.
+ rule_define ($1, $amfile, TARGET_USER, $cond || 'TRUE', $here);
+
check_variable_expansions ($_, $here);
$output_trailer .= $comment . $spacing;
@@ -7896,7 +8050,7 @@
foreach (split (' ' , $targets))
{
- # FIXME: We are not robust to people defining several targets
+ # FIXME: 1. We are not robust to people defining several targets
# at once, only some of them being in %dependencies. The
# actions from the targets in %dependencies are usually generated
# from the content of %actions, but if some targets in $targets
@@ -7904,13 +8058,16 @@
# a rule for all $targets (i.e. the targets which are both
# in %dependencies and $targets will have two rules).
- # FIXME: The logic here is not able to output a
+ # FIXME: 2. The logic here is not able to output a
# multi-paragraph rule several time (e.g. for each conditional
# it is defined for) because it only knows the first paragraph.
+ # FIXME: 3. We are not robust to people defining a subset
+ # of a previously defined "multiple-target" rule. E.g.
+ # `foo:' after `foo bar:'.
+
# Output only if not in FALSE.
- if (defined $dependencies{$_}
- && $cond ne 'FALSE')
+ if (defined $dependencies{$_} && $cond ne 'FALSE')
{
&depend ($_, @deps);
$actions{$_} .= $actions;
@@ -7919,55 +8076,22 @@
{
# Free-lance dependency. Output the rule for all the
# targets instead of one by one.
-
- # Work out all the conditions for which the target hasn't
- # been defined
- my @undefined_conds;
- if (defined $target_conditional{$targets})
- {
- my @defined_conds = keys %{$target_conditional{$targets}};
- @undefined_conds = invert_conditions(@defined_conds);
- }
- else
+ my @undefined_conds =
+ rule_define ($targets, $file,
+ $is_am ? TARGET_AUTOMAKE : TARGET_USER,
+ $cond || 'TRUE', $file);
+ for my $undefined_cond (@undefined_conds)
{
- if (defined $targets{$targets})
- {
- # No conditions for which target hasn't been defined
- @undefined_conds = ();
- }
- else
- {
- # Target hasn't been defined for any conditions
- @undefined_conds = ("");
- }
+ my $condparagraph = $paragraph;
+ $condparagraph =~ s/^/$undefined_cond/gm;
+ $result_rules .= "$spacing$comment$condparagraph\n";
}
-
- if ($cond ne 'FALSE')
+ if (scalar @undefined_conds == 0)
{
- for my $undefined_cond (@undefined_conds)
- {
- my $condparagraph = $paragraph;
- $condparagraph =~ s/^/make_condition (@cond_stack,
$undefined_cond)/gme;
- if (rule_define ($targets, $is_am,
- "$cond $undefined_cond", $file))
- {
- $result_rules .=
- "$spacing$comment$condparagraph\n"
- }
- else
- {
- # Remember to discard next paragraphs
- # if they belong to this rule.
- $discard_rule = 1;
- }
- }
- if ($#undefined_conds == -1)
- {
- # This target has already been defined, the rule
- # has not been defined. Remember to discard next
- # paragraphs if they belong to this rule.
- $discard_rule = 1;
- }
+ # Remember to discard next paragraphs
+ # if they belong to this rule.
+ # (but see also FIXME: #2 above.)
+ $discard_rule = 1;
}
$comment = $spacing = '';
last;
Index: tests/Makefile.am
===================================================================
RCS file: /cvs/automake/automake/tests/Makefile.am,v
retrieving revision 1.435
diff -u -r1.435 Makefile.am
--- tests/Makefile.am 10 Sep 2002 09:50:23 -0000 1.435
+++ tests/Makefile.am 10 Sep 2002 22:32:42 -0000
@@ -1,6 +1,7 @@
## Process this file with automake to create Makefile.in
-XFAIL_TESTS = subdir5.test auxdir2.test cond17.test
+XFAIL_TESTS = subdir5.test auxdir2.test cond17.test \
+ specflags7.test specflags8.test
TESTS = \
acinclude.test \
--
Alexandre Duret-Lutz
- RFC: diagnose target clashes (for PR/344),
Alexandre Duret-Lutz <=
- Re: RFC: diagnose target clashes (for PR/344), Richard Boulton, 2002/09/11
- Re: RFC: diagnose target clashes (for PR/344), Alexandre Duret-Lutz, 2002/09/11
- Re: RFC: diagnose target clashes (for PR/344), Tom Tromey, 2002/09/11
- Re: RFC: diagnose target clashes (for PR/344), Akim Demaille, 2002/09/13
- FYI: diagnose target clashes (for PR/344), Alexandre Duret-Lutz, 2002/09/13