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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Shane Caraveo (:mixedpuppy)]

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.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

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

[:Gijs (he/him)]

(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.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

(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.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

(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

[:Gijs (he/him)]

(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...

Comment 6

[:Gijs (he/him)]

(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...).

Comment 7

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e3c5b55659d3611d82ad2428389efeb9e27fefb

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1261456 - Combine support-files listed in [DEFAULT] with any listed per-test rather than overriding.

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/

Comment 10

[Gregory Szorc [: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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68779538bf00

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68779538bf00