diff --git a/NEWS b/NEWS index 571845dd..7ff24d2a 100644 --- a/NEWS +++ b/NEWS @@ -42,6 +42,11 @@ https://sv.gnu.org/bugs/index.php?group=make&report_id=111&fix_release_id=111&se "environment override" even if they are not otherwise set in the makefile. See https://savannah.gnu.org/bugs/index.php?64803 +* WARNING: Backward-incompatibility! + The behavior of appending to pattern-specific variables has been clarified + when combined with command-line settings or -e overrides. + See https://savannah.gnu.org/bugs/index.php?64822 + * NOTE: Deprecated behavior. The check in GNU Make 4.3 for suffix rules with prerequisites didn't check single-suffix rules, only double-suffix rules. Add the missing check. diff --git a/doc/make.texi b/doc/make.texi index 497230f7..ba1545f6 100644 --- a/doc/make.texi +++ b/doc/make.texi @@ -6466,8 +6466,9 @@ switches with a command argument just as usual. You could use this override CFLAGS += -g @end example -You can also use @code{override} directives with @code{define} directives. -This is done as you might expect: +You can also use @code{override} directives with @code{define} directives +(@pxref{Multi-Line, , Defining Multi-Line Variables}). This is done as you +might expect: @example override define foo = @@ -6475,14 +6476,6 @@ bar endef @end example -@noindent -@iftex -See the next section for information about @code{define}. -@end iftex -@ifnottex -@xref{Multi-Line, ,Defining Multi-Line Variables}. -@end ifnottex - @node Multi-Line @section Defining Multi-Line Variables @findex define diff --git a/src/expand.c b/src/expand.c index 66c55e95..253a588e 100644 --- a/src/expand.c +++ b/src/expand.c @@ -148,13 +148,14 @@ recursively_expand_for_file (struct variable *v, struct file *file) const floc **saved_varp; struct variable_set_list *savev = 0; int set_reading = 0; + size_t nl = strlen (v->name); + struct variable *parent = NULL; /* If we're expanding to put into the environment of a shell function then ignore any recursion issues: for backward-compatibility we will use the value of the environment variable we were started with. */ if (v->expanding && env_recursion) { - size_t nl = strlen (v->name); char **ep; DB (DB_VERBOSE, (_("%s:%lu: not recursively expanding %s to export to shell function\n"), @@ -203,8 +204,49 @@ recursively_expand_for_file (struct variable *v, struct file *file) v->expanding = 1; if (v->append) + { + /* Find a parent definition which is marked override. */ + struct variable_set_list *sl; + for (sl = current_variable_set_list; sl && !parent; sl = sl->next) + { + struct variable *vp = lookup_variable_in_set (v->name, nl, sl->set); + if (vp && vp != v && vp->origin == o_override) + parent = vp; + } + } + + if (parent) + /* PARENT is an override, V is appending. If V is also an override: + override hello := first + al%: override hello += second + Then construct the value from its appended parts in the parent sets. + Else if V is not an override: + override hello := first + al%: hello += second + Then ignore the value of V and use the value of PARENT. */ + value = v->origin == o_override + ? allocated_variable_append (v) + : xstrdup (parent->value); + else if (v->origin == o_command || v->origin == o_env_override) + /* Avoid appending to a pattern-specific variable, unless the origin of this + pattern-specific variable beats or equals the origin of one of the parent + definitions of this variable. + This is needed, because if there is a command line definition or an env + override, then the value defined in the makefile should only be appended + in the case of a file override. + In the presence of command line definition or env override and absence of + makefile override, the value should be expanded, rather than appended. In + this case, at parse time record_target_var already set the value of this + pattern-specific variable to the value defined on the command line or to + the env override value. + User provided a command line definition or an env override. + PARENT does not have an override directive, so ignore it. */ + value = allocated_expand_string (v->value); + else if (v->append) + /* Construct the value from its appended parts in the parent sets. */ value = allocated_variable_append (v); else + /* A definition without appending. */ value = allocated_expand_string (v->value); v->expanding = 0; diff --git a/src/variable.c b/src/variable.c index 2026e987..635d79d3 100644 --- a/src/variable.c +++ b/src/variable.c @@ -1432,29 +1432,56 @@ do_variable_definition (const floc *flocp, const char *varname, const char *valu case f_append: case f_append_value: { - /* If we have += but we're in a target variable context, we want to - append only with other variables in the context of this target. */ - if (scope) + int override = 0; + if (scope == s_global) + v = lookup_variable (varname, strlen (varname)); + else { + /* When appending in a target/pattern variable context, we want to + append only with other variables in the context of this + target/pattern. */ append = 1; v = lookup_variable_in_set (varname, strlen (varname), current_variable_set_list->set); + if (v) + { + /* Don't append from the global set if a previous non-appending + target/pattern-specific variable definition exists. */ + if (!v->append) + append = 0; - /* Don't append from the global set if a previous non-appending - target-specific variable definition exists. */ - if (v && !v->append) - append = 0; + if (scope == s_pattern && + (v->origin == o_env_override || v->origin == o_command)) + { + /* This is the case of multiple target/pattern specific + definitions/appends, e.g. + al%: hello := first + al%: hello += second + in the presence of a command line definition or an + env override. Do not merge x->value and value here. + For pattern-specific variables the values are merged in + recursively_expand_for_file. */ + override = 1; + append = 1; + } + } } - else - v = lookup_variable (varname, strlen (varname)); - if (v == 0) + if (!v) { - /* There was no old value. - This becomes a normal recursive definition. */ + /* There was no old value: make this a recursive definition. */ newval = value; flavor = f_recursive; } + else if (override) + { + /* Command line definition / env override takes precedence over + a pattern/target-specific append. */ + newval = value; + /* Set flavor to f_recursive to recursively expand this variable + at build time in recursively_expand_for_file. */ + flavor = f_recursive; + } else { /* Paste the old and new values together in VALUE. */ diff --git a/tests/scripts/variables/append b/tests/scripts/variables/append new file mode 100644 index 00000000..eae40e7f --- /dev/null +++ b/tests/scripts/variables/append @@ -0,0 +1,280 @@ +# -*-perl-*- + +use warnings; + +my $description = "Test appending to variables of various origins."; + +# sv 64822, sv 36486. +# Test various combinations of appends in the presence and absence of command +# line definitions, env overrides and makefile overrides. +# Test the following statements. +# A target or pattern specific definition or append is available at build time +# only. +# A definition with a makefile override can only be appended to with another +# override. +# A definition with a makefile override can only be redefined with another +# override. +# A target or pattern specific definition takes precedence over a global +# definition for that target. +# A global variable is immune to a target or pattern specific definition or +# append. +# A target or pattern specific definition is immune to another target or pattern +# specific definition or append. +# A prerequisite inherits a target or pattern specific value from its target. +# "phobos" inherits the value of "hello" from "mars". + +my @goals = ('phobos', 'mars'); +my @specificities = ('', 's: ', '%: '); +my @inits = ('', 'hello=file', 'hello:=file', 'override hello=file'); +my @global_append = ('', 'hello+=red'); +my @blue_override = ('', 'override '); +my @yellow_override = ('', 'override '); +my @cmdline = ('', 'hello=cmd', 'hello:=cmd hello+=cmd2'); +my @env_ovr = ('', '-e'); +my @envs = ('', 'env'); + +for my $goal (@goals) { + for my $spec (@specificities) { + for my $init (@inits) { + for my $ga (@global_append) { + for my $bovr (@blue_override) { + for my $yovr (@yellow_override) { + for my $dashe (@env_ovr) { + for my $env (@envs) { + for my $cmd (@cmdline) { + + if ($env) { + $ENV{'hello'} = 'env'; + } else { + delete $ENV{'hello'}; + } + + my $parse_time_answer = ''; + my $build_time_answer = ''; + + # For goal "phobos" target is "", "phobos: " and "phobo%: ". + # For goal "mars" target is "", "mars: " and "mar%: ". + my $target = ''; + if ($spec) { + $target = $goal; + chop($target); + $target .= $spec; + } + + if ($init =~ 'override') { + $build_time_answer = 'file'; + } elsif ($cmd) { + $build_time_answer = $cmd =~ 'cmd2' ? 'cmd cmd2' : 'cmd'; + } elsif ($dashe and $env) { + $build_time_answer = 'env'; + } elsif ($init and !$target and $ga) { + $build_time_answer = 'file red'; + } elsif ($init) { + $build_time_answer = 'file'; + } elsif ($env and $ga) { + $build_time_answer = 'env red'; + } elsif ($env) { + $build_time_answer = 'env'; + } elsif ($ga) { + $build_time_answer = 'red'; + } + + if ($bovr and $yovr) { + $build_time_answer .= ' ' if ($build_time_answer); + $build_time_answer .= 'blue yellow'; + } elsif ($bovr) { + $build_time_answer .= ' ' if ($build_time_answer); + $build_time_answer .= 'blue'; + } elsif ($yovr and !($init =~ 'override') and !$cmd and !($dashe and $env)) { + $build_time_answer .= ' ' if ($build_time_answer); + $build_time_answer .= 'blue yellow'; + } elsif ($yovr) { + $build_time_answer .= ' ' if ($build_time_answer); + $build_time_answer .= 'yellow'; + } elsif ($init =~ 'override') { + } elsif ($cmd) { + } elsif ($dashe and $env) { + } else { + $build_time_answer .= ' ' if ($build_time_answer); + $build_time_answer .= 'blue yellow'; + } + + + if ($cmd and $target) { + $parse_time_answer = $cmd =~ 'cmd2' ? 'cmd cmd2' : 'cmd'; + } elsif ($env and !$dashe and $target and $ga) { + $parse_time_answer = 'env red'; + } elsif ($env and $target) { + $parse_time_answer = 'env'; + } elsif ($target and $ga) { + $parse_time_answer = 'red'; + } elsif ($target) { + $parse_time_answer = ''; + } else { + $parse_time_answer = $build_time_answer; + } + + my $i = $init ? "$target$init" : ''; + + # moon and satur% specific settings test that target and pattern + # settings specific to one target do not affect another target. + + my $answer = $goal eq "mars" ? + "$parse_time_answer\n$build_time_answer\n" # From parse time and from "phobos" recipe. + . "$build_time_answer\n#MAKE#: 'mars' is up to date.\n" : # From "mars" recipe. + "$parse_time_answer\n$build_time_answer\n#MAKE#: 'phobos' is up to date.\n"; + + + run_make_test(" +# goal = $goal +# init = $init +# target = $target +# ga = $ga +# bovr = $bovr +# yovr = $yovr +# cmd = $cmd +# env = $env +# dashe = $dashe + +moon: hello=1 +moon: override hello+=2 + +$i +$ga +$target${bovr}hello+=blue +$target${yovr}hello+=yellow + +satur%: override hello:=3 +satur%: hello+=4 + +\$(info \$(hello)) +phobos:; \$(info \$(hello)) +mars: phobos; \$(info \$(hello)) +saturn:; \$(info \$(hello)) +", "$dashe $cmd $goal", "$answer"); + } + } + } + } + } + } + } + } +} + +# Preferably, we would want to run the tests below for all the combinations, +# generated by the loop above. However, that causes the test to take a lot of +# time. + +# The fix for sv 64822 went to recursively_expand_for_file. +# There are three code paths that lead to recursively_expand_for_file. +# - export of a variable to target environment. +# - expansion of a substitution reference. +# - other expansions of a variable that was appended to. +# Test target env, substitution reference in parse and build modes.; +# Also test a mix of pattern and target specific definitions and appends. + +run_make_test(q! +al%: hello=file +al%: hello+=one +all: hello+=two +$(info $(hello)) +all:; $(info $(hello)) +!, "", "\nfile one two\n#MAKE#: 'all' is up to date.\n"); + +run_make_test(q! +hello=file +al%: hello+=one +all: hello+=two +$(info $(hello:X=Y)) +all:; $(info $(hello:X=Y)) +!, "", "file\nfile one two\n#MAKE#: 'all' is up to date.\n"); + +run_make_test(q! +hello=file +al%: hello+=one +all: hello+=two +export hello +$(info $(shell echo $$hello)) +all:; $(info $(shell echo $$hello)) +!, "", "file\nfile one two\n#MAKE#: 'all' is up to date.\n"); + +# "phobos" inherits the value of "hello" from "mars". On top of that there are +# also "phobos" specific appends. +for my $goal (@goals) { +my $answer = $goal eq "mars" ? + "\nminit mone mtwo pone ptwo\n" # From parse time and from "phobos" recipe. + . "minit mone mtwo\n#MAKE#: 'mars' is up to date.\n" : # From "mars" recipe. + "\npone ptwo\n#MAKE#: 'phobos' is up to date.\n"; # From parse time and from "phobos" recipe. + +run_make_test(" +mar%: hello=minit +mar%: hello+=mone +mars: hello+=mtwo +phobo%: hello+=pone +phobos: hello+=ptwo +\$(info \$(hello)) +phobos:; \$(info \$(hello)) +mars: phobos; \$(info \$(hello)) +", "$goal", "$answer"); +} + +# This test is similar to the one above. The difference is that here there is a +# pattern-specific definition of "hello" that matches "phobos". +run_make_test(q! +mar%: hello:=minit +mar%: hello+=mone +mars: hello+=mtwo +phobo%: hello:=pinit +phobo%: hello+=pone +phobos: hello+=ptwo +$(info $(hello)) +phobos:; $(info $(hello)) +mars: phobos; $(info $(hello)) +!, 'phobos', "\npinit pone ptwo\n#MAKE#: 'phobos' is up to date.\n"); + +# Test pattern and target specific appends to a global variable that has origin override. +# sv 36486. +my @ops = ('=', '+=', ':='); +@inits = ('', 'override ', 'al%: override '); +@specificities = ('', 'all: ', 'al%: '); +for my $init (@inits) { + for my $spec (@specificities) { + for my $op (@ops) { + + my $build_time_answer = ''; + if ($init =~ ':' and $op eq '+=' and !$spec) { + # This is the case where global variable obtains value 'one two three' at + # parse time and later at build time a pattern or target specific + # 'hello+=file' appends 'file'. + # e.g. + # al%: override hello+=file + # hello+=one + # hello+=two + # hello+=three + $build_time_answer = 'one two three file'; + } elsif ($init =~ 'override') { + $build_time_answer = 'file'; + } else { + $build_time_answer = 'file one two three'; + } + + my $parse_time_answer = $init =~ ':' ? '' : 'file'; + if (!$spec and ($init ne 'override ')) { + $parse_time_answer .= ' ' if $parse_time_answer; + $parse_time_answer .= 'one two three'; + } + + run_make_test(" +${init}hello${op}file +${spec}hello+=one +${spec}hello+=two +${spec}hello+=three +\$(info \$(hello)) +all:; \$(info \$(hello)) +", "", "$parse_time_answer\n$build_time_answer\n#MAKE#: 'all' is up to date.\n"); + } + } +} + +1;