Closed Bug 994255 Opened 6 years ago Closed 3 years ago

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

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: mossop, Assigned: kmag)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
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.
Pretty sure this is a dupe. Does the manifest parser expose included manifests yet? That was the blocker before.
I thought billm fixed this in the "run mochitests from manifests" bug, but maybe not.
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?)
Flags: needinfo?(gps)
testing/mozbase/manifestparser/
Flags: needinfo?(gps)
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...
Depends on: 1113644
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
https://hg.mozilla.org/mozilla-central/rev/1386141ac9f7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla38 → ---
I just hit this in bug 1214058.
Assignee: nobody → kmaglione+bmo
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
Attachment #8812379 - Flags: review?(gps) → review+
(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 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.
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/59f2e54d4122
Add included and parent test manifest files to backend inputs list. r=gps
https://hg.mozilla.org/mozilla-central/rev/59f2e54d4122
Status: REOPENED → RESOLVED
Closed: 5 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1341502
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.