994255 - Changing an included xpcshell manifest doesn't cause mach to detect the changes

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dave Townsend [:mossop]]

In the add-on manager tests xpcshell.ini and xpcshell-unpack.ini both include xpcshell-shared.ini. I added my test to xpcshell-shared.ini, did a full rebuild but it still didn't pick up the new test. Touching the root manifests did the trick though.

Comment 1

[Dave Townsend [:mossop]]

Similarly if I remove a test file and remove it from the shared manifest subsequent attempts to rebuild just complain that the target of the symlink doesn't exist until I touch the parent manifests.

Comment 2

[Gregory Szorc [:gps]]

Pretty sure this is a dupe. Does the manifest parser expose included manifests yet? That was the blocker before.

Comment 3

[(not currently active) Ted Mielczarek]

I thought billm fixed this in the "run mochitests from manifests" bug, but maybe not.

Comment 4

[:Gijs (he/him)]

Having just spent 10 minutes trying to figure out why my test wasn't running...

(In reply to Gregory Szorc [:gps] from comment #2)
> Pretty sure this is a dupe. Does the manifest parser expose included
> manifests yet? That was the blocker before.

How do I find out the answer to this question so that we can get traction here? (ie what is this parser and/or where does it live?)

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

testing/mozbase/manifestparser/

Comment 6

[:Gijs (he/him)]

Looks like 'no', then:

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/manifestparser.py#489

I don't know how to interrogate history there (the m-c bit is just a "mirror this from github", and the github repo has been cleared out, AFAICT), and/or if a bug got filed for it...

Comment 7

[Dave Townsend [:mossop]]

Yeah so we have to do fun things like this in order to land new tests for the add-ons manager now: https://hg.mozilla.org/integration/fx-team/rev/1386141ac9f7

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1386141ac9f7

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c950de0ffffb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c950de0ffffb

Comment 11

[Kris Maglione [:kmag]]

I just hit this in bug 1214058.

Comment 12

[Kris Maglione [:kmag]]

Attachment: Bug 994255: Add included and parent test manifest files to backend inputs list.

Review commit: https://reviewboard.mozilla.org/r/94146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94146/

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8812379 [details]
Bug 994255: Add included and parent test manifest files to backend inputs list.

https://reviewboard.mozilla.org/r/94146/#review94756

Aside from a few nits (which shouldn't matter), this is good. I triggered a Try push just in case this trips up existing tests.

::: python/mozbuild/mozbuild/backend/recursivemake.py:1049
(Diff revision 1)
>          self.backend_input_files.add(mozpath.join(obj.topsrcdir,
>              obj.manifest_relpath))

These lines can be safely deleted since the main manifest will be in `obj.source_relpaths`, right?

::: python/mozbuild/mozbuild/frontend/data.py:653
(Diff revision 1)
>          # manifestparser.TestManifest of the other one.
>          'dupe_manifest',
>      )
>  
>      def __init__(self, context, path, manifest, flavor=None,
> -            install_prefix=None, relpath=None, dupe_manifest=False):
> +            install_prefix=None, relpath=None, sources=(),

Nit: Don't use lists, tuples, dicts, or other non-scalar types as default argument values in Python because they don't do what you expect.

http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments

Comment 14

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #13)
> >      def __init__(self, context, path, manifest, flavor=None,
> > -            install_prefix=None, relpath=None, dupe_manifest=False):
> > +            install_prefix=None, relpath=None, sources=(),
> 
> Nit: Don't use lists, tuples, dicts, or other non-scalar types as default
> argument values in Python because they don't do what you expect.
> 
> http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-
> arguments

Tuples are immutable.

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8812379 [details]
Bug 994255: Add included and parent test manifest files to backend inputs list.

https://reviewboard.mozilla.org/r/94146/#review94756

> These lines can be safely deleted since the main manifest will be in `obj.source_relpaths`, right?

Yeah. I was actually on the fence about removing them.

> Nit: Don't use lists, tuples, dicts, or other non-scalar types as default argument values in Python because they don't do what you expect.
> 
> http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments

I went with a tuple because they're immutable, and treated more or less like scalars.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8812379 [details]
Bug 994255: Add included and parent test manifest files to backend inputs list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94146/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59f2e54d4122
Add included and parent test manifest files to backend inputs list. r=gps

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59f2e54d4122