Build system should combine [default] and per-test support-files when installing support files

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: mixedpuppy, Assigned: chmanchester)

Tracking

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
Using the following browser.ini file (below), only test.html is placed into the test directory, the default section is ignored.  This caused the bc5 failure here:

https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=9b5113e833ff005d4845e88d8de1041985a8c5c7&selectedJob=8365737



[DEFAULT]
support-files =
  head.js

[browser_pocket_ui_check.js]
support-files = test.html


I didn't catch it because initially test.html was in the default section and it worked as expected.  I moved test.html to where it is shown above, rebuilt, ran tests and it worked as expected.  Only by doing a fresh build did I see that the default section was ignored.
Thanks for filing. This has always been the behavior of support-files (the per-test entry overwrites the entry in DEFAULT). It's not great, but most manifests in the tree get away with it because they'll have some files that have their own support-files entry, and some that don't, and because we install support files for every test when any test runs, we get the files installed anyway.

There's a change in the works (bug 1242051) that changes some things about how this works, but hasn't landed yet, and would not impact this specific issue.

Comment 2

3 years ago
(In reply to Chris Manchester (:chmanchester) from comment #1)
> Thanks for filing. This has always been the behavior of support-files (the
> per-test entry overwrites the entry in DEFAULT).

Can you point out where this happens? I looked for a bit, but couldn't find the relevant code.
Flags: needinfo?(cmanchester)
(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Chris Manchester (:chmanchester) from comment #1)
> > Thanks for filing. This has always been the behavior of support-files (the
> > per-test entry overwrites the entry in DEFAULT).
> 
> Can you point out where this happens? I looked for a bit, but couldn't find
> the relevant code.

It's in https://dxr.mozilla.org/mozilla-central/rev/55d557f4d73ee58664bdf2fa85aaab555224722e/testing/mozbase/manifestparser/manifestparser/ini.py#72

The "current_section" variable is a dictionary that gets updates, there's no notion of appending to the existing value.
Flags: needinfo?(cmanchester)
(In reply to Chris Manchester (:chmanchester) from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > (In reply to Chris Manchester (:chmanchester) from comment #1)
> > > Thanks for filing. This has always been the behavior of support-files (the
> > > per-test entry overwrites the entry in DEFAULT).
> > 
> > Can you point out where this happens? I looked for a bit, but couldn't find
> > the relevant code.
> 
> It's in
> https://dxr.mozilla.org/mozilla-central/rev/
> 55d557f4d73ee58664bdf2fa85aaab555224722e/testing/mozbase/manifestparser/
> manifestparser/ini.py#72
> 
> The "current_section" variable is a dictionary that gets updates, there's no
> notion of appending to the existing value.

Actually, it looks like it does implement combining values, but only for 'skip-if'.

Comment 5

3 years ago
(In reply to Chris Manchester (:chmanchester) from comment #4)
> (In reply to Chris Manchester (:chmanchester) from comment #3)
> > (In reply to :Gijs Kruitbosch from comment #2)
> > > (In reply to Chris Manchester (:chmanchester) from comment #1)
> > > > Thanks for filing. This has always been the behavior of support-files (the
> > > > per-test entry overwrites the entry in DEFAULT).
> > > 
> > > Can you point out where this happens? I looked for a bit, but couldn't find
> > > the relevant code.
> > 
> > It's in
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 55d557f4d73ee58664bdf2fa85aaab555224722e/testing/mozbase/manifestparser/
> > manifestparser/ini.py#72
> > 
> > The "current_section" variable is a dictionary that gets updates, there's no
> > notion of appending to the existing value.
> 
> Actually, it looks like it does implement combining values, but only for
> 'skip-if'.

Right. I tried adding an analogous fix there, but that breaks because software is awful. Let me explain in more detail:

1. the final error message indicates we're installing the same stuff multiple times.
2. this is because now of course the default files are included for each test
3. ... but wait, that already happened here: https://dxr.mozilla.org/mozilla-central/rev/9bd90088875399347b05d87c67d3709e31539dcd/testing/mozbase/manifestparser/manifestparser/manifestparser.py#176-178
4. ... but that specific case (no support-files for a test, but we do have some in the [DEFAULT] section) is caught here:
https://dxr.mozilla.org/mozilla-central/rev/9bd90088875399347b05d87c67d3709e31539dcd/python/mozbuild/mozbuild/frontend/emitter.py#1103-1114

so what started as a performance fix is now actively interfering with this working.

Of course, then yesterday bug 1242051 landed and those dxr links no longer match up with my source tree, so I need to go digging where that logic moved to...
Depends on: 1242051

Comment 6

3 years ago
(In reply to :Gijs Kruitbosch from comment #5)
> Of course, then yesterday bug 1242051 landed and those dxr links no longer
> match up with my source tree, so I need to go digging where that logic moved
> to...

It seems this now lives in python/mozbuild/mozbuild/testing.py, but the logic has changed slightly and I don't know if it's safe to ignore previously processed things in there - it seems like that will make the outcome of the method vary depending on whether we're processing the entire manifest or just a single test - at least the commit message for https://hg.mozilla.org/integration/fx-team/rev/e174b6bf5439 says that this code needs to support that case. Plus, the straightforward way of fixing this would be to just stick all the .split()'d values in the "seen" set, but I don't know to what degree that would break existing behaviour.

Chris, thoughts? Am I missing the point? (I'm almost hoping I am...).
Flags: needinfo?(cmanchester)

Updated

3 years ago
Summary: support-files failure → Build system should combine [default] and per-test support-files when installing support files
There's no real magic here, we just want to prevent listing the same files multiple times to keep cruft down, and the underlying facility for installing the files (install manifests) does not tolerate duplicates.

Changing the way we memoize by doing so on the basis of individual files in a field (instead of the literal string that composes the field, causing the weirdness in comment 5) should work fine.

And now that I'm thinking about it, figuring out the right thing to do here shouldn't be too hard, so why don't I take a stab at it.
Assignee: nobody → cmanchester
Flags: needinfo?(cmanchester)
This requires a change to how we process test manifests in the build system:
now, whenever we see a support file mentioned in a manifest, we require that
file isn't already in that test's support files, but if we see a support file
that was already seen in some other test, the entry is ignored, but it is not
an error. As a result of this change, several duplicate support-files entries
needed to be removed.

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

Updated

3 years ago
Attachment #8739225 - Flags: review?(gps)
Comment on attachment 8739225 [details]
MozReview Request: Bug 1261456 - Combine support-files listed in [DEFAULT] with any listed per-test rather than overriding.

https://reviewboard.mozilla.org/r/45125/#review41739

Nice improvement.

::: python/mozbuild/mozbuild/testing.py:347
(Diff revision 1)
> -            value = test.get(thing, '')
> +            value = test.get(field, '')
> +            for pattern in value.split():
> +
> +                # We track uniqueness locally (per test) where it is forbidden, and globally,
> +                # where it is permitted. If a support file appears multiple times for a single
> +                # test, there are unecessary entries in the manifest. But many entries will be

unnecessary
Attachment #8739225 - Flags: review?(gps) → review+

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/68779538bf00
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.