Closed Bug 952564 Opened 6 years ago Closed 4 years ago

Disallow empty lists in moz.build variables

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: gps, Assigned: sambuddhabasu1, Mentored)

Details

(Whiteboard: [lang=python])

Attachments

(2 files, 3 obsolete files)

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.
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.
Whiteboard: [mentor=gps][lang=python]
Can i pick this Bug?
(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.
Flags: needinfo?(adarshdinesh)
self.assertTrue() check for lists will be enough ?
Flags: needinfo?(adarshdinesh)
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).
<         import assert
..
<         self.assertGreater(len(_execution_stack),0);
I think we should add this checking just before the _execution_stack.pop() ,right ?
Flags: needinfo?
I think it should come between sandbox.exec_file() and the self._sandbox_post_eval_cb(sandbox) call.
Flags: needinfo?
Attached patch firstpatch.diff (obsolete) — Splinter Review
Please review my first Patch..
Attachment #8359923 - Flags: review?(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...
Attachment #8359923 - Flags: review?(gps)
Attached patch patch_two(18-01-14).diff (obsolete) — Splinter Review
1) Added a checking for empty lists in 'sandbox'.
2) Raising the already existing SandboxValidationError.
Attachment #8359923 - Attachment is obsolete: true
Attachment #8362053 - Flags: review?(gps)
Please let me know if anything else to be done. Currently the bug is assigned to "nobody@mozilla.org" please assign it to me.
Hope the Attachment #8362053 [details] [diff] will fix the issue please review it.
I've been swamped with things lately. This isn't the only patch I haven't reviewed quickly. I apologize for the delay.
Assignee: nobody → adarshdinesh
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
Attachment #8362053 - Flags: review?(gps) → feedback+
Adding the line DIRS = [] in moz.build raises the error "Empty list in moz.build variable". 

Please review the patch.
Attachment #8362053 - Attachment is obsolete: true
Attachment #8388185 - Flags: review?(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
Attachment #8388185 - Flags: review?(gps) → feedback+
Mentor: gps
Whiteboard: [mentor=gps][lang=python] → [lang=python]
If no one is presently working on this bug, I would like to work on it.
@sambudda: sorry I was a little busy with my work. Yes, you can work on this bug. Please talk with @gps
Status: NEW → ASSIGNED
Assignee: adarshdinesh → sambuddhabasu1
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.
Attachment #8719922 - Flags: review?(gps) → review+
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.
Attachment #8719922 - Flags: review+
Attachment #8719922 - Attachment is obsolete: true
Attachment #8720089 - Flags: review?(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.
Attachment #8720089 - Flags: review?(gps) → review+
I tweaked the error message before landing to sound slightly more natural.

Thanks for the patch!
https://hg.mozilla.org/mozilla-central/rev/12149cb75011
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.