Re: [PATCH 2/2] order_only prereq shall not use parent per_target variables

[Bernhard Reutner-Fischer]

On September 16, 2014 10:59:44 AM GMT+02:00, Bernhard Reutner-Fischer 
<address@hidden> wrote:
>On 16 September 2014 06:21, Paul Smith <address@hidden> wrote:
>> On Mon, 2014-09-15 at 13:03 +0200, Bernhard Reutner-Fischer wrote:
>>> Order-only prerequisites should not use the per_target variables of
>it's
>>> parent.
>>>
>>> given:
>>> obj.o: X=-Dobj
>>> crt1.o:
>>> obj.o: | crt1.o
>>>
>>> we do not want crt1.o to be built with X set.
>>
>> I haven't applied this change yet: this is a change in the current
>> functionality of GNU make and it's not clear to me why
>target-specific
>> variables should work this way rather than the way they do now.
>>
>> I can see arguments both ways.  Why shouldn't an order-only
>prerequisite
>> take target-specific values from its parent?

See below for my reasoning.
OK?

TIA and cheers,

>
>Because an order-only prerequisite is, well, order-only? :)
>
>My reasoning is that per-target variables are to be used only for the
>target
>they are defined for, not for random other targets, especially not for
>order-only targets.
>You would not want a per-target append variable to alter upper-scopes
>value
>either (i.e. not -Udir1 -Udir2 in one cc invocation in the sample
>below).
>
>How do you suggest would i build crt1.o above without X being in the
>scope
>of crt1.o?
>
>The only working workaround i can see is spawning a separate instance,
>but
>that is something i really do not want to do:
>
>$ /usr/bin/make -f GNUmakefile.workaround workaround=workaround
>/usr/bin/make -f GNUmakefile.workaround crt1.o
>make[1]: Entering directory '/scratch/src/bug-make'
>cc -c a.c -o crt1.o
>make[1]: Leaving directory '/scratch/src/bug-make'
>cc -Udir1 -c a.c -o dir1/file1.o -Ddir1
>cc -Udir2 -c a.c -o dir2/file1.o -Ddir2
>$
>Sample once again:
>---8<---
>.SUFFIXES:
>MAKEFLAGS += -r
>default: dir1/file1.o dir2/file1.o
># workaround is either 'workaround' or 'crt1.o'
>workaround ?= crt1.o
>workaround:
>        $(MAKE) -f $(word 1,$(MAKEFILE_LIST)) crt1.o
>dir1/file1.o dir2/file1.o: | $(workaround)
>CFLAGS-dir1 := -Ddir1
>CFLAGS-dir2 := -Ddir2
>doit = @echo $(CC) $(CFLAGS) -c a.c -o $@ $(PER_DIR)
>define add_per_dir
>ifneq ($(strip $(2)),)
>__add := $(2)
>$$(__add): PER_DIR:=$$(CFLAGS-$(1))
>$$(__add): CFLAGS+=-U$(1)
>endif
>endef
>$(eval $(call add_per_dir,dir1,dir1/file1.o))
>$(eval $(call add_per_dir,dir2,dir2/file1.o))
>%.o:
>        $(doit)
>        $(if $(PER_DIR),$(if $(findstring crt1,$*),false
>PER_DIR=$(PER_DIR) but should not be set for $*))
>---8<---
>
>See what i mean?
>thanks,
>>
>>
>>> Signed-off-by: Bernhard Reutner-Fischer <address@hidden>
>>> ---
>>>  tests/scripts/features/targetvars | 92
>+++++++++++++++++++++++++++++++++++++++
>>>  variable.c                        | 20 ++++++++-
>>>  2 files changed, 110 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/scripts/features/targetvars
>b/tests/scripts/features/targetvars
>>> index a9b8dbe..c1337e2 100644
>>> --- a/tests/scripts/features/targetvars
>>> +++ b/tests/scripts/features/targetvars
>>> @@ -270,4 +270,96 @@ a: ; @echo $(A)
>>>  # ',
>>>  #               '', "local\n");
>>>
>>> +# TEST #21: per-target variable should not be inherited by
>order-only
>>> +#           prerequisite
>>> +$details = "\
>>> +Per-target variable should not be inherited by order-only
>prerequisite";
>>> +
>>> +$makefile = &get_tmpfile;
>>> +unlink $makefile if -f $makefile;
>>> +
>>> +open(MAKEFILE,"> $makefile");
>>> +
>>> +print MAKEFILE <<'EOF';
>>> +.SUFFIXES:
>>> +default: dir1/file1.o dir2/file1.o | dir3/file3.o
>>> +
>>> +CFLAGS := -O2
>>> +CFLAGS-dir1 := -Ddir1
>>> +CFLAGS-dir2 := -Ddir2
>>> +doit = @echo $(CC) -c a.c -o $@ $(CFLAGS) $(PER_DIR)
>>> +define add_per_dir
>>> +ifneq ($(strip $(2)),)
>>> +__add := $(2)
>>> +$$(__add): PER_DIR:=$$(CFLAGS-$(1))
>>> +endif
>>> +endef
>>> +$(eval $(call add_per_dir,dir1,dir1/file1.o))
>>> +$(eval $(call add_per_dir,dir2,dir2/file1.o))
>>> +%.o:
>>> +     $(doit)
>>> +     @#$(if $(PER_DIR),@test "$*" != "dir3/file3" || false
>PER_DIR=$(PER_DIR) but should not be set)
>>> +ifndef OK
>>> +dir1/file1.o: | dir3/file3.o
>>> +endif
>>> +EOF
>>> +
>>> +close(MAKEFILE);
>>> +
>>> +# Variant 1
>>> +&run_make_with_options($makefile, "", &get_logfile);
>>> +$answer = "cc -c a.c -o dir3/file3.o -O2\ncc -c a.c -o dir1/file1.o
>-O2 -Ddir1\ncc -c a.c -o dir2/file1.o -O2 -Ddir2\n";
>>> +&compare_output($answer,&get_logfile(1));
>>> +
>>> +# Variant 2
>>> +&run_make_with_options($makefile, "OK=1", &get_logfile);
>>> +$answer = "cc -c a.c -o dir1/file1.o -O2 -Ddir1\ncc -c a.c -o
>dir2/file1.o -O2 -Ddir2\ncc -c a.c -o dir3/file3.o -O2\n";
>>> +&compare_output($answer,&get_logfile(1));
>>> +
>>> +# TEST #22: per-target variable should not be inherited by
>order-only
>>> +#           prerequisite
>>> +
>>> +$makefile = &get_tmpfile;
>>> +unlink $makefile if -f $makefile;
>>> +open(MAKEFILE,"> $makefile");
>>> +
>>> +print MAKEFILE <<'EOF';
>>> +.SUFFIXES:
>>> +default: libdir1.so libdir2.so | dir3/file3.o
>>> +
>>> +CFLAGS := -O2
>>> +CFLAGS-dir1 := -Ddir1
>>> +CFLAGS-dir2 := -Ddir2
>>> +doit = @echo $(CC) -c a.c -o $@ $(CFLAGS) $(PER_DIR)
>>> +linkit = @echo $(CC) -o $@ dir3/file3.o $^ $(CFLAGS) $(PER_DIR)
>>> +define add_per_dir
>>> +ifneq ($(strip $(2)),)
>>> +__add := $(2)
>>> +$$(__add): PER_DIR:=$$(CFLAGS-$(1))
>>> +endif
>>> +endef
>>> +$(eval $(call add_per_dir,dir1,dir1/file1.o))
>>> +$(eval $(call add_per_dir,dir2,dir2/file1.o))
>>> +lib%.so: %/file1.o | dir3/file3.o
>>> +     $(linkit)
>>> +%.o:
>>> +     $(doit)
>>> +     @#$(if $(PER_DIR),@test "$*" != "dir3/file3" || false
>PER_DIR=$(PER_DIR) but should not be set)
>>> +ifndef OK
>>> +dir1/file1.o: | dir3/file3.o
>>> +endif
>>> +EOF
>>> +
>>> +close(MAKEFILE);
>>> +
>>> +# Variant 3
>>> +&run_make_with_options($makefile, "", &get_logfile);
>>> +$answer = "cc -c a.c -o dir3/file3.o -O2\ncc -c a.c -o dir1/file1.o
>-O2 -Ddir1\ncc -o libdir1.so dir3/file3.o dir1/file1.o -O2\ncc -c a.c
>-o dir2/file1.o -O2 -Ddir2\ncc -o libdir2.so dir3/file3.o dir2/file1.o
>-O2\n";
>>> +&compare_output($answer,&get_logfile(1));
>>> +
>>> +# Variant 4
>>> +&run_make_with_options($makefile, "OK=1", &get_logfile);
>>> +$answer = "cc -c a.c -o dir1/file1.o -O2 -Ddir1\ncc -c a.c -o
>dir3/file3.o -O2\ncc -o libdir1.so dir3/file3.o dir1/file1.o -O2\ncc -c
>a.c -o dir2/file1.o -O2 -Ddir2\ncc -o libdir2.so dir3/file3.o
>dir2/file1.o -O2\n";
>>> +&compare_output($answer,&get_logfile(1));
>>> +
>>>  1;
>>> diff --git a/variable.c b/variable.c
>>> index 3f57e7d..39dca10 100644
>>> --- a/variable.c
>>> +++ b/variable.c
>>> @@ -565,8 +565,24 @@ initialize_file_variables (struct file *file,
>int reading)
>>>      l->next = &global_setlist;
>>>    else
>>>      {
>>> -      initialize_file_variables (file->parent, reading);
>>> -      l->next = file->parent->variables;
>>> +      struct dep *d;
>>> +      for (d = file->parent->deps; d; d = d->next)
>>> +     {
>>> +       if (d->file == file)
>>> +         /* Found the dep in our parent that points to ourself.  */
>>> +         break;
>>> +     }
>>> +      if (d && d->ignore_mtime)
>>> +     {
>>> +       /* We are an order_only prerequisite of our parent.
>>> +        * We are in no way interested in their per_target
>variables.  */
>>> +       l->next = &global_setlist;
>>> +     }
>>> +      else
>>> +     {
>>> +          initialize_file_variables (file->parent, reading);
>>> +          l->next = file->parent->variables;
>>> +     }
>>>      }
>>>    l->next_is_parent = 1;
>>>
>>
>>