922685 - pymake referencing wrong variable during $(foreach), possibly other functions

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

In the course of investigating bug 909508, I found a bug in pymake.

config/makefiles/nonrecursive.mk contains the following:

  $(foreach target,$(NONRECURSIVE_TARGETS),...)

The symptoms of bug 909508 indicated that the action in this loop wasn't executing properly.

I replaced the action with an $(info). e.g.

  $(foreach target,$(NONRECURSIVE_TARGETS),$(info $(target))

pymake printed "x86_64-apple-darwin12.5.0" instead of the expected value of "export". Digging deeper, "x86_64-apple-darwin12.5.0" is the value of "target" from config.status's subst dict, which is populated in make via config/autoconf.mk.

So, uh, it appears pymake's variable referencing in at least $(foreach) isn't properly obtaining the "local" variable over the global one.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Automatic variable references aren't handled properly

This test case reproduces the bug.

Comment 2

[Gregory Szorc [:gps]]

So this is a bug in pymake.data.Variables.set(). $(foreach) is setting the variable as source == automatic. This has lower priority than source == makefile, so the set is being ignored.

Looking at the $(origin) function (https://www.gnu.org/software/make/manual/make.html#Origin-Function), it isn't clear what type of variable the $(foreach) bound variable should be. If we write a trivial make file and execute it through GNU make, it reports "automatic."

So, pymake is doing the right thing in theory, it just doesn't actually set the variable because of priority issues.

I think we need an argument to force pymake to accept the variable set. Furthermore, we need to try..finally around the $(foreach) to restore the old value after execution.

Fun times.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Automatic variable references aren't handled properly

This appears to fix it.

We don't need a try..finally on the bound variable as I implied in my
last comment because pymake.data.Variables operates as a stack of
variables and evaluation of the $(foreach) allocates a new instance for
locals.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Local $(foreach) variable isn't set properly

Now with more comments. Sorry for the review spam.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 812704 [details] [diff] [review]
Local $(foreach) variable isn't set properly

Review of attachment 812704 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/pymake/pymake/functions.py
@@ +653,5 @@
> +            # conform with GNU make. However, automatic variables have low
> +            # priority. So, we must force its assignment to occur.
> +            v.set(vname, data.Variables.FLAVOR_SIMPLE,
> +                    data.Variables.SOURCE_AUTOMATIC, w, force=True)
> +            e.resolve(makefile, v, fd, settingf

It looks like you have a typo at the end of this line.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/rev/166a45b029b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f90880fb5248

with typo fixed. Derp.

I accidentally committed to inbound with r=bsmedberg. Unless he cares, I won't recommit.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f90880fb5248