Variable key replaces original warnings

Variable key replaces original warnings

[Olof Johansson]

Hi all,

We've started to see issues on master like

   WARNING:
     Variable key          USERADD_PARAM_${PN}             (; --system --home / --no-create-home --comment 'Storage manager daemon' --gid storage storage)
     replaces original key USERADD_PARAM_recording-indexer (--system --home / --no-create-home --comment 'apache httpd' --gid www www).

(my alignment...)

The warning happens when doing

 WWWUSER_PACKAGE ?= "${PN}"
 USERADD_PARAM_${WWWUSER_PACKAGE} := "--system --home / --no-create-home --gid www www"

 USERADD_PARAM_${PN}_append = "; --system --home / --no-create-home --gid username username"

(in reality, the first two lines are inherited from a class, and the third in
the recipe itself.)

With bitbake -e I see what looks like the expected value, with both users being
created. If I change USERADD_PARAM_${WWWUSER_PACKAGE} (second line) to
USERADD_PARAM_${PN} no warning is printed, so this only seems to be a problem
in cases where multiple unique non-expanded vars expand to the same expanded
var.

I made a unit test that should demonstrate the problem (I think), and it fails
because of the warnings:

diff --git a/bitbake/lib/bb/tests/data.py b/bitbake/lib/bb/tests/data.py
index e9aab57..e7716cc 100644
--- a/bitbake/lib/bb/tests/data.py
+++ b/bitbake/lib/bb/tests/data.py
@@ -386,6 +386,15 @@ class TestKeyExpansion(unittest.TestCase):
             self.assertTrue(logContains("Variable key VAL_${FOO} (A) replaces original key VAL_foo (B)", logs))
         self.assertEqual(self.d.getVar("VAL_foo", True), "A")
 
+    def test_append(self):
+        self.d.setVar("TEST_${BAR}", "Bar")
+        self.d.setVar("TEST_${FOO}_append", "Foo")
+        with LogRecord() as logs:
+            bb.data.expandKeys(self.d)
+            self.assertFalse(logContains("Variable key TEST_${FOO} (Foo) replaces original key TEST_foo (Bar)", logs))
+        self.assertEqual(self.d.getVar("TEST_${FOO}", True), "BarFoo")


If you have any ideas on what this issue can be or how to fix it, please
let me know :)

-- 
olof johansson

Re: Variable key replaces original warnings

[Richard Purdie]

On Thu, 2015-07-30 at 17:03 +0200, Olof Johansson wrote:
> Hi all,
> 
> We've started to see issues on master like
> 
>    WARNING:
>      Variable key          USERADD_PARAM_${PN}             (; --system --home / --no-create-home --comment 'Storage manager daemon' --gid storage storage)
>      replaces original key USERADD_PARAM_recording-indexer (--system --home / --no-create-home --comment 'apache httpd' --gid www www).
> 
> (my alignment...)
> 
> The warning happens when doing
> 
>  WWWUSER_PACKAGE ?= "${PN}"
>  USERADD_PARAM_${WWWUSER_PACKAGE} := "--system --home / --no-create-home --gid www www"
> 
>  USERADD_PARAM_${PN}_append = "; --system --home / --no-create-home --gid username username"
> 
> (in reality, the first two lines are inherited from a class, and the third in
> the recipe itself.)
> 
> With bitbake -e I see what looks like the expected value, with both users being
> created. If I change USERADD_PARAM_${WWWUSER_PACKAGE} (second line) to
> USERADD_PARAM_${PN} no warning is printed, so this only seems to be a problem
> in cases where multiple unique non-expanded vars expand to the same expanded
> var.
> 
> I made a unit test that should demonstrate the problem (I think), and it fails
> because of the warnings:
> 
> diff --git a/bitbake/lib/bb/tests/data.py b/bitbake/lib/bb/tests/data.py
> index e9aab57..e7716cc 100644
> --- a/bitbake/lib/bb/tests/data.py
> +++ b/bitbake/lib/bb/tests/data.py
> @@ -386,6 +386,15 @@ class TestKeyExpansion(unittest.TestCase):
>              self.assertTrue(logContains("Variable key VAL_${FOO} (A) replaces original key VAL_foo (B)", logs))
>          self.assertEqual(self.d.getVar("VAL_foo", True), "A")
>  
> +    def test_append(self):
> +        self.d.setVar("TEST_${BAR}", "Bar")
> +        self.d.setVar("TEST_${FOO}_append", "Foo")
> +        with LogRecord() as logs:
> +            bb.data.expandKeys(self.d)
> +            self.assertFalse(logContains("Variable key TEST_${FOO} (Foo) replaces original key TEST_foo (Bar)", logs))
> +        self.assertEqual(self.d.getVar("TEST_${FOO}", True), "BarFoo")
> 
> 
> If you have any ideas on what this issue can be or how to fix it, please
> let me know :)

The data store changes recently caused a number of these to appear. The
issue has always been there, as has this warning, the new datastore code
just exposes it a bit more consistently than previously.

The reason we do this is that users with conflicting (different values)
of the same variable name usually have a metadata issue they're unaware
of.

The fix is to ensure you only set a variable in one canonical way.
OE-Core did show a few of these but I fixed them before merging the data
store changes.

Cheers,

Richard

Re: Variable key replaces original warnings

[Richard Purdie]

On Fri, 2015-07-31 at 10:54 +0100, Richard Purdie wrote:
> On Thu, 2015-07-30 at 17:03 +0200, Olof Johansson wrote:
> > Hi all,
> > 
> > We've started to see issues on master like
> > 
> >    WARNING:
> >      Variable key          USERADD_PARAM_${PN}             (; --system --home / --no-create-home --comment 'Storage manager daemon' --gid storage storage)
> >      replaces original key USERADD_PARAM_recording-indexer (--system --home / --no-create-home --comment 'apache httpd' --gid www www).
> > 
> > (my alignment...)
> > 
> > The warning happens when doing
> > 
> >  WWWUSER_PACKAGE ?= "${PN}"
> >  USERADD_PARAM_${WWWUSER_PACKAGE} := "--system --home / --no-create-home --gid www www"
> > 
> >  USERADD_PARAM_${PN}_append = "; --system --home / --no-create-home --gid username username"
> > 
> > (in reality, the first two lines are inherited from a class, and the third in
> > the recipe itself.)
> > 
> > With bitbake -e I see what looks like the expected value, with both users being
> > created. If I change USERADD_PARAM_${WWWUSER_PACKAGE} (second line) to
> > USERADD_PARAM_${PN} no warning is printed, so this only seems to be a problem
> > in cases where multiple unique non-expanded vars expand to the same expanded
> > var.
> > 
> > I made a unit test that should demonstrate the problem (I think), and it fails
> > because of the warnings:
> > 
> > diff --git a/bitbake/lib/bb/tests/data.py b/bitbake/lib/bb/tests/data.py
> > index e9aab57..e7716cc 100644
> > --- a/bitbake/lib/bb/tests/data.py
> > +++ b/bitbake/lib/bb/tests/data.py
> > @@ -386,6 +386,15 @@ class TestKeyExpansion(unittest.TestCase):
> >              self.assertTrue(logContains("Variable key VAL_${FOO} (A) replaces original key VAL_foo (B)", logs))
> >          self.assertEqual(self.d.getVar("VAL_foo", True), "A")
> >  
> > +    def test_append(self):
> > +        self.d.setVar("TEST_${BAR}", "Bar")
> > +        self.d.setVar("TEST_${FOO}_append", "Foo")
> > +        with LogRecord() as logs:
> > +            bb.data.expandKeys(self.d)
> > +            self.assertFalse(logContains("Variable key TEST_${FOO} (Foo) replaces original key TEST_foo (Bar)", logs))
> > +        self.assertEqual(self.d.getVar("TEST_${FOO}", True), "BarFoo")
> > 
> > 
> > If you have any ideas on what this issue can be or how to fix it, please
> > let me know :)
> 
> The data store changes recently caused a number of these to appear. The
> issue has always been there, as has this warning, the new datastore code
> just exposes it a bit more consistently than previously.
> 
> The reason we do this is that users with conflicting (different values)
> of the same variable name usually have a metadata issue they're unaware
> of.
> 
> The fix is to ensure you only set a variable in one canonical way.
> OE-Core did show a few of these but I fixed them before merging the data
> store changes.

The OE-Core change I made for a similar case was:

http://git.yoctoproject.org/cgit.cgi/poky/commit/meta/classes/update-rc.d.bbclass?id=3b627bb28c4ea2ea33050ad4884c6351e2d6ebad

i.e. moved to anonymous python. Not ideal, but more explicit.

Cheers,

Richard

Re: Variable key replaces original warnings

[Olof Johansson]

Excerpts from Richard Purdie's message of 2015-07-31 11:54:02 +0200:
> On Thu, 2015-07-30 at 17:03 +0200, Olof Johansson wrote:
> > I made a unit test that should demonstrate the problem (I think), and it fails
> > because of the warnings:
> > 
> > diff --git a/bitbake/lib/bb/tests/data.py b/bitbake/lib/bb/tests/data.py
> > index e9aab57..e7716cc 100644
> > --- a/bitbake/lib/bb/tests/data.py
> > +++ b/bitbake/lib/bb/tests/data.py
> > @@ -386,6 +386,15 @@ class TestKeyExpansion(unittest.TestCase):
> >              self.assertTrue(logContains("Variable key VAL_${FOO} (A) replaces original key VAL_foo (B)", logs))
> >          self.assertEqual(self.d.getVar("VAL_foo", True), "A")
> >  
> > +    def test_append(self):
> > +        self.d.setVar("TEST_${BAR}", "Bar")
> > +        self.d.setVar("TEST_${FOO}_append", "Foo")
> > +        with LogRecord() as logs:
> > +            bb.data.expandKeys(self.d)
> > +            self.assertFalse(logContains("Variable key TEST_${FOO} (Foo) replaces original key TEST_foo (Bar)", logs))
> > +        self.assertEqual(self.d.getVar("TEST_${FOO}", True), "BarFoo")
> > 
> > 
> > If you have any ideas on what this issue can be or how to fix it, please
> > let me know :)
> 
> The data store changes recently caused a number of these to appear. The
> issue has always been there, as has this warning, the new datastore code
> just exposes it a bit more consistently than previously.
> 
> The reason we do this is that users with conflicting (different values)
> of the same variable name usually have a metadata issue they're unaware
> of.

Thanks. But that's not the case for us. Why is this relevant for _append
operations? The exanded variable does get the expected value (which can
be verified by uncommenting the assertFalse for the log in my unit
test).

> The fix is to ensure you only set a variable in one canonical way.

Why? For our purpose that's not ideal. We want to be able to create a
user with a common definition from a set of packages. It's not
necessarily the recipes' ${PN} package that will create such a user.

-- 
olof johansson

Re: Variable key replaces original warnings

[Olof Johansson]

Excerpts from Richard Purdie's message of 2015-07-31 12:00:06 +0200:
> The OE-Core change I made for a similar case was:
> 
> http://git.yoctoproject.org/cgit.cgi/poky/commit/meta/classes/update-rc.d.bbclass?id=3b627bb28c4ea2ea33050ad4884c6351e2d6ebad
> 
> i.e. moved to anonymous python. Not ideal, but more explicit.

Ah, thanks. Didn't see this before replying to your other mail. But I
agree, not ideal at all. It's very surprising. What I would expect is
that the _append is postponed and only done on the expanded vars, but
that's not the case? Why not?

-- 
olof johansson

Re: Variable key replaces original warnings

[Richard Purdie]

On Fri, 2015-07-31 at 13:48 +0200, Olof Johansson wrote:
> Excerpts from Richard Purdie's message of 2015-07-31 12:00:06 +0200:
> > The OE-Core change I made for a similar case was:
> > 
> > http://git.yoctoproject.org/cgit.cgi/poky/commit/meta/classes/update-rc.d.bbclass?id=3b627bb28c4ea2ea33050ad4884c6351e2d6ebad
> > 
> > i.e. moved to anonymous python. Not ideal, but more explicit.
> 
> Ah, thanks. Didn't see this before replying to your other mail. But I
> agree, not ideal at all. It's very surprising. What I would expect is
> that the _append is postponed and only done on the expanded vars, but
> that's not the case? Why not?

Its all about the timing of the override expansion (_append is
effectively an override) and the key expansion. Taking:

PN = A
RRECOMMENDS_${PN} = "X" 
UPDATERCPN = "${PN}" 
RRECOMMENDS_${UPDATERCPN}_append = "Y"

if you expand overrides first it changes to:

RRECOMMENDS_${PN} = "X" 
UPDATERCPN = "${PN}" 
RRECOMMENDS_${UPDATERCPN} = "Y"

and it then becomes:

RRECOMMENDS_A = "Y"

however if you do key expansion first it becomes:

RRECOMMENDS_A = "X" 
RRECOMMENDS_A_append = "Y"

so RRECOMMENDS_A = "XY" 

We're now using the former rather than the latter in bitbake however it
will generate warnings where it sees variables conflict since users tend
not to realise and mean something different.

Worst case we could make the warnings configurable or whitelist them but
simply changing the code not to be ambiguous is likely the best all
around solution.

Cheers,

Richard