946175 - Forbid overwriting variable in moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Typical mistake that can happen in a moz.build:

FOO = ['foo']
FOO = ['bar'] instead of FOO += ['bar']

We discussed preventing those kind of errors on irc some time ago.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Make all sandbox variables default to an instance of their class type

I think this makes things cleaner, and it helps me in bug 945042.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Forbid assigning over a value previously set in moz.build

The open question is what to do with export()ed variables (see the unittest changes). Either we treat them specially and allow the first assignment to them in a subdirectory, or, like this patch does, we don't and it's impossible to change the value of an exported variable from a subdirectory's moz.build. Note that we currently don't rely on this being possible on m-c (no idea about c-c).

I have another patch which also rejects direct assignments for StrictOrderingOnAppendLists and HierarchicalStringLists, so that doing e.g.

  EXPORTS.mozilla = ['foo.h']
or
  SOURCES = ['bar.cpp']

is forbidden (use += instead). I'm not entirely sure we need this. I'll happily finish it (it requires changing a lot of moz.build files that do assign to those lists) if you think it's worth it.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8342298 [details] [diff] [review]
Make all sandbox variables default to an instance of their class type

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

I /think/ we still iterate over the sandbox symbols somewhere in |mach build-docs| land and we'll get a tuple length mismatch when running |mach build-docs| with this patch.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8342301 [details] [diff] [review]
Forbid assigning over a value previously set in moz.build

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

What's the output of config.status when this failure occurs? Is the error message as intuitive as existing error messages strive to be? I wonder if we should have a follow-up to raise a dedicated exception class so error reporting can report it better.

::: python/mozbuild/mozbuild/frontend/sandbox.py
@@ +159,5 @@
> +        #   foo.__iadd__(['bar'])
> +        #   namespace.__setitem__('FOO', foo)
> +        # This means __setitem__ is called with the value that is already
> +        # in the dict, when doing +=, which is permitted.
> +        if name in self and not dict.__getitem__(self, name) is value:

Use the "is not" operator.

::: python/mozbuild/mozbuild/test/frontend/test_namespaces.py
@@ +80,5 @@
>  
>          self.assertEqual(ke.exception.args[1], 'set_unknown')
>  
> +        with self.assertRaises(Exception) as ke:
> +            ns['foo'] = False

Let's check the error message. There's an assertRaisesRegex or something to make this easy.

::: toolkit/components/diskspacewatcher/moz.build
@@ -12,5 @@
>      'DiskSpaceWatcher.h'
>  ]
>  
>  XPIDL_MODULE = 'diskspacewatcher'
> -XPIDL_MODULE = 'toolkitcomps'

Wat.

Comment 5

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Created attachment 8342301 [details] [diff] [review]
> Forbid assigning over a value previously set in moz.build
> 
> The open question is what to do with export()ed variables (see the unittest
> changes). Either we treat them specially and allow the first assignment to
> them in a subdirectory, or, like this patch does, we don't and it's
> impossible to change the value of an exported variable from a subdirectory's
> moz.build. Note that we currently don't rely on this being possible on m-c
> (no idea about c-c).

I'm fine with leaving this future footgun in the implementation. export() is already kinda hacky anyway. Simple now, complex later.

> I have another patch which also rejects direct assignments for
> StrictOrderingOnAppendLists and HierarchicalStringLists, so that doing e.g.
> 
>   EXPORTS.mozilla = ['foo.h']
> or
>   SOURCES = ['bar.cpp']
> 
> is forbidden (use += instead). I'm not entirely sure we need this. I'll
> happily finish it (it requires changing a lot of moz.build files that do
> assign to those lists) if you think it's worth it.

I'm not going to ask for it. I'll review it if I see a patch.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #4)
> Comment on attachment 8342301 [details] [diff] [review]
> Forbid assigning over a value previously set in moz.build
> 
> Review of attachment 8342301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What's the output of config.status when this failure occurs?

ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file:

    /home/mh/mozilla-central/toolkit/components/diskspacewatcher/moz.build

The error was triggered on line 16 of this file:

    XPIDL_MODULE = 'toolkitcomps'

An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

The error as reported by Python is:

    ['Exception: Reassigning XPIDL_MODULE is forbidden\n']

Comment 7

[Mike Hommey [:glandium]]

Attachment: Make all sandbox variables default to an instance of their class type

Unbreaking mach build-docs

Comment 8

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #4)
> ::: toolkit/components/diskspacewatcher/moz.build
> @@ -12,5 @@
> >      'DiskSpaceWatcher.h'
> >  ]
> >  
> >  XPIDL_MODULE = 'diskspacewatcher'
> > -XPIDL_MODULE = 'toolkitcomps'
> 
> Wat.

Comes from bug 939044.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Interdiff that needs to be folded in the second part

Nick for the header change. Greg for the rest.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8344429 [details] [diff] [review]
Interdiff that needs to be folded in the second part

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

::: python/mozbuild/mozbuild/frontend/sandbox.py
@@ +209,5 @@
>          self._allow_all_writes = True
>          yield self
>          self._allow_all_writes = False
>  
> +    def update(self, other):

Please document why this is required.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Interdiff that needs to be folded in the second part

The header part is actually for bug 945042

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8344435 [details] [diff] [review]
Interdiff that needs to be folded in the second part

Carrying over r+

Comment 13

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/e926070b0d18
https://hg.mozilla.org/mozilla-central/rev/c9d4612aef4c