Closed Bug 946175 Opened 6 years ago Closed 6 years ago

Forbid overwriting variable in moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
I think this makes things cleaner, and it helps me in bug 945042.
Attachment #8342298 - Flags: review?(gps)
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.
Attachment #8342301 - Flags: review?(gps)
Blocks: 945042
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.
Attachment #8342298 - Flags: review?(gps) → review+
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.
Attachment #8342301 - Flags: review?(gps) → review+
(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.
(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']
Attachment #8342298 - Attachment is obsolete: true
(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.
Nick for the header change. Greg for the rest.
Attachment #8344429 - Flags: review?(n.nethercote)
Attachment #8344429 - Flags: review?(gps)
Attachment #8343552 - Flags: review?(gps) → review+
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.
Attachment #8344429 - Flags: review?(gps) → review+
Attachment #8344429 - Attachment is obsolete: true
Attachment #8344429 - Flags: review?(n.nethercote)
Comment on attachment 8344435 [details] [diff] [review]
Interdiff that needs to be folded in the second part

Carrying over r+
Attachment #8344435 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e926070b0d18
https://hg.mozilla.org/mozilla-central/rev/c9d4612aef4c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.