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

[Bernhard Reutner-Fischer]

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?

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;
>>
>
>