952564 - Disallow empty lists in moz.build variables

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

As part of bug 951352, we found a moz.build file that assigned an empty list. No clue how that happened. We should nip the practice in the bud.

Potential issue: post-process sandbox callbacks. If we do this validation in emitter.py, some lists that are reset by post-read sandbox callbacks may trigger this failure. We may have to perform this check in reader.py, before those callbacks are performed.

Comment 1

[Gregory Szorc [:gps]]

Code is in python/mozbuild/mozbuild/frontend in mozilla-central. You probably want to add a check somewhere in the _read_mozbuild() function to look for empty lists.

Comment 2

[adu]

Can i pick this Bug?

Comment 3

[Gregory Szorc [:gps]]

(In reply to adu from comment #2)
> Can i pick this Bug?

Yes you can! I saw you pinged me in IRC but you logged off before I read it. Try pinging again or let me know in this bug what information you need to be unblocked from starting on it.

Comment 4

[adu]

self.assertTrue() check for lists will be enough ?

Comment 5

[Gregory Szorc [:gps]]

Code in reader.py should raise a SandboxValidationException if lists are empty. There should be a simple test to ensure this exception is raised if lists are empty. (self.assertRaises or self.assertRaisesRegexp).

Comment 6

[adu]

<         import assert
..
<         self.assertGreater(len(_execution_stack),0);
I think we should add this checking just before the _execution_stack.pop() ,right ?

Comment 7

[Gregory Szorc [:gps]]

I think it should come between sandbox.exec_file() and the self._sandbox_post_eval_cb(sandbox) call.

Comment 8

[adu]

Attachment: firstpatch.diff

Please review my first Patch..

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8359923 [details] [diff] [review]
firstpatch.diff

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

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +311,5 @@
>      pass
>  
> +class SandboxValidationException(Exception):
> +    """Empty list Exception."""
> +    pass

There is already a SandboxValidationError class. You should use that.

@@ +730,5 @@
>          sandbox.exec_file(path, filesystem_absolute=filesystem_absolute)
>          sandbox.execution_time = time.time() - time_start
> +        
> +        if not len(self._execution_stack):
> +            raise SandboxValidationException

Nit: Leading whitespace.

You aren't checking the proper thing. You'll need to iterate over the variables inside the sandbox:

    for k in sandbox:
        v = sandbox[k]
        # if v is a list and it is empty...

Comment 10

[adu]

Attachment: patch_two(18-01-14).diff

1) Added a checking for empty lists in 'sandbox'.
2) Raising the already existing SandboxValidationError.

Comment 11

[adu]

Please let me know if anything else to be done. Currently the bug is assigned to "[e-mail]" please assign it to me.
Hope the Attachment #8362053 [details] [diff] will fix the issue please review it.

Comment 12

[Gregory Szorc [:gps]]

I've been swamped with things lately. This isn't the only patch I haven't reviewed quickly. I apologize for the delay.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8362053 [details] [diff] [review]
patch_two(18-01-14).diff

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

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +729,5 @@
>  
> +        for k in sandbox:
> +	    v = sandbox[k];
> +	    if not len(v) and type(v)==list :
> +                raise SandboxValidationError

Use isinstance(v, type) for testing whether something is a specific type.

You should also do the type checking before len() because len() may not work on all object types! I think this patch may have gotten lucky!

So, we have multiple list types in our sandbox. There's likely a followup here to ensure other types are caught as well. I think we can ignore that for now, however.

You also need to pass an argument to SandboxValidationError.

::: python/mozbuild/mozbuild/test/frontend/test_reader.py
@@ +282,5 @@
>          self.assertEqual(len(sandboxes), 1)
>          self.assertEqual(len(count), 1)
>  
> +    def test_empty_list(self):
> +        reader = self.reader('reader-empty-list')

Did you forget to commit some new files?

@@ +288,5 @@
> +        with self.assertRaises(SandboxValidationError) as bre:
> +            list(reader._read_mozbuild())
> +        e = bre.exception
> +        self.assertIn('The underlying problem is an empty list',
> +            str(e))

Did you actually run the tests? This message doesn't appear in your patch and I'm sure this would fail. You can run the tests by running:

$ ./mach python-test python/mozbuild/mozbuild/test/frontend

Comment 14

[adu]

Attachment: patch_9_Mar_2014.diff

Adding the line DIRS = [] in moz.build raises the error "Empty list in moz.build variable". 

Please review the patch.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8388185 [details] [diff] [review]
patch_9_Mar_2014.diff

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

First, I truly apologize for not reviewing this promptly. I've been spending a lot of my time trying to land a feature in time for Firefox 30 and haven't had time for many things outside of that.

Aside from the test issue, the patch as-is is sound. We could still check for empty lists in other types like HierarchicalStringList. I'm thinking we could do something like:

if hasattr(v, 'validate'):
    v.validate()

Then we can sprinkle validate() methods on custom types to perform type-specific validation inside the class definition.

But that can be relegated to a follow-up.

Also, this patch breaks the build because there are empty lists in the wild! You'll need to fix those as part of this patch.

I took the liberty of changing your patch to emit just a warning and I pushed it to our automation at https://tbpl.mozilla.org/?tree=Try&rev=971def7b2c10. You'll want to look the full logs for every job, search for "Empty list in moz.build variable" and then fix all reported problems. I don't think there will be too many.

Again looking. Good. Just needs a bit more work. Ignore the comments about validate() unless you really want to tackle that.

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +764,5 @@
>          sandbox.execution_time = time.time() - time_start
>  
> +        for k in sandbox:
> +            v = sandbox[k]
> +            if isinstance(v,list) and not len(v):

Nit: |not len(v)| is the equivalent of |not v|

::: python/mozbuild/mozbuild/test/frontend/data/reader-error-empty-list/moz.build
@@ +1,4 @@
> +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> +# Any copyright is dedicated to the Public Domain.
> +# http://creativecommons.org/publicdomain/zero/1.0/
> +

There is no content in this file and this causes the test to fail:

TEST-UNEXPECTED-FAIL | /Users/gps/src/firefox/python/mozbuild/mozbuild/test/frontend/test_reader.py | line 288, test_empty_list: SandboxValidationError not raised

Comment 16

[Sambuddha Basu [:sambuddhabasu1]]

If no one is presently working on this bug, I would like to work on it.

Comment 17

[adu]

@sambudda: sorry I was a little busy with my work. Yes, you can work on this bug. Please talk with @gps

Comment 18

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Disallow empty lists in moz.build variables

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8719922 [details] [diff] [review]
Disallow empty lists in moz.build variables

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

Very nice. I'll check this in for you.

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8719922 [details] [diff] [review]
Disallow empty lists in moz.build variables

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

It looks like `mach configure` fails on Linux with this patch:

 0:03.38 mozbuild.frontend.reader.BuildReaderError:
 0:03.38 ==============================
 0:03.38 ERROR PROCESSING MOZBUILD FILE
 0:03.38 ==============================
 0:03.38
 0:03.38 The error occurred while processing the following file:
 0:03.38
 0:03.38     /home/gps/src/firefox/mozglue/build/moz.build
 0:03.38
 0:03.38 The error occurred when validating the result of the execution. The reported error is:
 0:03.38
 0:03.38     Variable LDFLAGS assigned to an empty list.
 0:03.38

I got to the 3rd error before I gave up. That last failure was in mozglue/tests/moz.build and due to LDFLAGS being empty. However, the content of this moz.build is:

  GeckoCppUnitTests([
      'ShowSSEConfig',
  ], linkage=None)

So, this will require some changes to templates to fix. The fix should be as simple as adding "if" checks before += operations to verify the right hand side isn't empty. Hopefully we only need to do this in <10 places. If we need to do it a lot, we should reconsider the approach in this bug because excessive "if" checks feel like an anti-pattern to me.

Comment 21

[Mike Hommey [:glandium]]

This probably only needs a change here:
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/reader.py#424

Comment 22

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Disallow empty lists in moz.build variables

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8720089 [details] [diff] [review]
Disallow empty lists in moz.build variables

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

Looks good! I'll land this for you.

Comment 24

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12149cb75011fb342d6808f0554b9391666f9d1a
Bug 952564 - Disallow empty lists in moz.build variables r=gps

Comment 25

[Gregory Szorc [:gps]]

I tweaked the error message before landing to sound slightly more natural.

Thanks for the patch!

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12149cb75011