1644223 - ancestor_manifest does not work on Windows: ancestor defaults are not inherit

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Rob Wu [:robwu]]

I'm mentoring a bug from a contributor, and they reported not being able to run tests, here: https://phabricator.services.mozilla.com/D66732#2395941

The log file shows const _HEAD_FILES = [] in the command, which is strange because the test manifest is referenced by an ancestor test manifest that does define head files. When I run the test locally on Linux and macOS, the test runs just fine.

I asked for the pickled test manifests, and got them here: https://phabricator.services.mozilla.com/D66732#2396314
The relevant parts for the analysis of this bug are included below (formatted with python -c 'import pickle;import sys;import pprint;pprint.pprint(pickle.load(open(sys.argv[1], "rb")))' all-tests.pkl , and also for test-defaults.pkl ).

From the given pickle, I can see:

• ancestor_manifest is a relative path using backslashes (Windows' path separator)
• manifest is an absolute path using backslashes (Windows' path separator)

The relevant part of manifest_defaults reader in BuildBackendLoader converts ancestor_manifest to an absolute path using os.path.join(self.topsrcdir, ancestor_manifest), and then uses the manifest key to look up defaults.

• os.path.join uses the platform's path separator (backslashes).
• ancestor_manifest was already using backslashes.
• ... but self.topsrcdir is inherit ... from MozbuildObject, which uses self.topsrcdir = mozpath.normsep(topsrcdir), which returns a path with forward slashes instead of the platform's default. Consequently, the generated path to the ancestor manifest is not recognized, and the defaults are not inherit.

The parser should be fixed, to consistently use backslashes (or forward slashes, but certainly not both).

all-tests.pkl

 'toolkit/components/extensions/test/xpcshell/test_ext_webRequest_download.js': [{'ancestor_manifest': 'toolkit\\components\\extensions\\test\\xpcshell\\xpcshell-remote.ini',
                                                                                  'dir_relpath': 'toolkit/components/extensions/test/xpcshell',
                                                                                  'file_relpath': 'toolkit/components/extensions/test/xpcshell/test_ext_webRequest_download.js',
                                                                                  'flavor': 'xpcshell',
                                                                                  'manifest': 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\xpcshell-common.ini',
                                                                                  'manifest_relpath': 'toolkit/components/extensions/test/xpcshell/xpcshell-common.ini',
                                                                                  'name': 'test_ext_webRequest_download.js',
                                                                                  'path': 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\test_ext_webRequest_download.js',
                                                                                  'relpath': 'toolkit\\components\\extensions\\test\\xpcshell\\test_ext_webRequest_download.js',
                                                                                  'srcdir_relpath': 'toolkit/components/extensions/test/xpcshell/test_ext_webRequest_download.js'},
                                                                                 {'ancestor_manifest': 'toolkit\\components\\extensions\\test\\xpcshell\\xpcshell.ini',
                                                                                  'dir_relpath': 'toolkit/components/extensions/test/xpcshell',
                                                                                  'file_relpath': 'toolkit/components/extensions/test/xpcshell/test_ext_webRequest_download.js',
                                                                                  'flavor': 'xpcshell',
                                                                                  'manifest': 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\xpcshell-common.ini',
                                                                                  'manifest_relpath': 'toolkit/components/extensions/test/xpcshell/xpcshell-common.ini',
                                                                                  'name': 'test_ext_webRequest_download.js',
                                                                                  'path': 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\test_ext_webRequest_download.js',
                                                                                  'relpath': 'toolkit\\components\\extensions\\test\\xpcshell\\test_ext_webRequest_download.js',
                                                                                  'srcdir_relpath': 'toolkit/components/extensions/test/xpcshell/test_ext_webRequest_download.js'}],

test-defaults.pkl:

'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\xpcshell-remote.ini': {'dupe-manifest': '',
                                                                                                               'firefox-appdir': 'browser',
                                                                                                               'head': 'head.js '
                                                                                                                       'head_remote.js '
                                                                                                                       'head_e10s.js '
                                                                                                                       'head_telemetry.js',
                                                                                                               'here': 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell',
...
'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\xpcshell.ini': {'dupe-manifest': '',
                                                                                                        'firefox-appdir': 'browser',
                                                                                                        'head': 'head.js '
                                                                                                                'head_telemetry.js',
                                                                                                        'here': 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell',
...
('c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\xpcshell-remote.ini', 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\xpcshell-common.ini'): {'here': 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell'},
...
('c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\xpcshell.ini', 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell\\xpcshell-common.ini'): {'here': 'c:\\mozilla-source\\mozilla-central\\toolkit\\components\\extensions\\test\\xpcshell',

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1644223 - Avoid unnecessary rebuild in test_resolve.py

test_resolve.py spends too much time on running. This is because the
backend was regenerated using data from the whole tree, at every test
despite the test fixture.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1644223 - Add unit test to verify that manifest_defaults are correctly read from test-defaults.pkl

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1644223 - Fix path normalization to correctly inherit manifest defaults on Windows

Note: the bug is fixed by the change in resolve.py.

manifestparser.py has no behavioral changes. It was refactored to make
the type (i.e. normalized absolute path) of parentmanifest more
obvious, and as a nice bonus the manifest_relpath value is generated
only once, instead of anew for every section in the test manifest.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ce76bf4aa57
Avoid unnecessary rebuild in test_resolve.py r=gbrown
https://hg.mozilla.org/integration/autoland/rev/c69a7026968b
Add unit test to verify that manifest_defaults are correctly read from test-defaults.pkl r=gbrown
https://hg.mozilla.org/integration/autoland/rev/ffd3e1dc8f38
Fix path normalization to correctly inherit manifest defaults on Windows r=gbrown

Comment 5

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1ce76bf4aa57
https://hg.mozilla.org/mozilla-central/rev/c69a7026968b
https://hg.mozilla.org/mozilla-central/rev/ffd3e1dc8f38

Comment 6

[Rob Wu [:robwu]]

Comment on attachment 9155314 [details]
Bug 1644223 - Fix path normalization to correctly inherit manifest defaults on Windows

Beta/Release Uplift Approval Request

• User impact if declined: Does not affect Firefox users but Firefox developers. Firefox Developers on Windows cannot run a specific unit test that is declared in a test manifest that is included by another test manifest.
  Since 78 is going to become ESR, I'd like to fix this so that Windows developers won't be tripped by this bug when they need to run a test on the upcoming ESR78.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1644793
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Test-only changes. These patches fix a defect by only changing a very narrow, specific part of the internal representation of the affected feature.
  There was a test failure in a test that checked internal state, but that was fixed in bug 1644793 (which should also be uplifted).
• String changes made/needed:

Comment 7

[Tomislav Jovanovic :zombie]

You should emphasize that all these are test only changes, and thus probably don't even need product approval, though I'm not sure we can land things directly to beta/esr without the formality of one.

Comment 8

[Rob Wu [:robwu]]

(In reply to Tomislav Jovanovic :zombie from comment #7)

You should emphasize that all these are test only changes, and thus probably don't even need product approval, though I'm not sure we can land things directly to beta/esr without the formality of one.

Thanks. I updated the comment to emphasize that the "user" is a "developer" and that these changes are test-only.

Comment 9

[Julien Cristau [:jcristau] (back August 18)]

https://hg.mozilla.org/releases/mozilla-beta/rev/38b4fec0a110
https://hg.mozilla.org/releases/mozilla-beta/rev/5b56f6dc6afb
https://hg.mozilla.org/releases/mozilla-beta/rev/93907044ac6a