[manifestparser] test relpaths not calculated properly with the mozbuild model (no root manifest)

RESOLVED FIXED in Firefox 39

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
This was a fun one to debug. First some back story.

In bug 1131098 I'm trying to move chunking algorithms out of mochitest and into manifestparser. One of those algorithms (chunk_by_dir) requires the relative path of each test to the source of the repo.

I noticed that each test object's 'relpath' attribute was simply the file name of the test. I also noticed some extra non-standard attributes like 'file_relpath' and 'dir_relpath' (these extra attributes were added by mozbuild [1], likely because the standard 'relpath' attribute wasn't doing the right thing). Now 'file_relpath' was exactly what I needed, but unfortunately I couldn't use it because it is non-standard and the chunking algorithm lives in manifestparser core. So I did some more investigation.

The manifestparser docs state that 'relpath' is the "relative path starting from the root manifest location". This is accomplished by setting manifest.rootdir to the directory of the first manifest file passed in (aka the "root" manifest) [2]. But in the case of mochitests (and others) there is no root manifest. Instead the build system is creating a new TestManifest object for each manifest in the tree. The rootdir is therefore set to the directory of each test manifest in turn, and relpath is calculated as just the test's file name (as more often than note, tests live in the same directory as their associated manifests).

The solution is to tweak the definition of relpath a little. In the case of mochitest/xpcshell/etc 'relpath' should be the path relative to the root of the repository (gecko), since there is no root manifest. This can be accomplished by passing the root repository path in to TestManifest.__init__().

This sounds confusing and complicated, but the patch is quite simple. Will upload in a bit after some more testing.

[1] https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/common.py#168
[2] https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/manifestparser.py?from=manifestparser.py#228
(Assignee)

Comment 1

4 years ago
Created attachment 8568815 [details]
MozReview Request: bz://1134395/ahal

/r/4275 - Bug 1134395 - mozbuild should pass in rootdir to manifestparser to properly calculate test relpaths, r=gps

Pull down this commit:

hg pull review -r 9853847b7c79b9360f5d5870d7359e7c9e362f94
(Assignee)

Comment 2

4 years ago
Comment on attachment 8568815 [details]
MozReview Request: bz://1134395/ahal

/r/4275 - Bug 1134395 - mozbuild should pass in rootdir to manifestparser to properly calculate test relpaths, r=gps

Pull down this commit:

hg pull review -r 9853847b7c79b9360f5d5870d7359e7c9e362f94
Attachment #8568815 - Flags: review?(gps)

Comment 3

4 years ago
https://reviewboard.mozilla.org/r/4275/#review3633

Touching this code always scares me. But this change looks good to me.

Comment 4

4 years ago
Comment on attachment 8568815 [details]
MozReview Request: bz://1134395/ahal

https://reviewboard.mozilla.org/r/4273/#review3635

Ship It!
Attachment #8568815 - Flags: review?(gps) → review+
(Assignee)

Comment 5

4 years ago
(In reply to Gregory Szorc [:gps] from comment #3)
> Touching this code always scares me. But this change looks good to me.

I spent a fair amount of time smoothing over some issues I initially caused. Here are some passing try runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=001da02ebe73
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ed201d01fa3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8250f4a84e81
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 8

3 years ago
Comment on attachment 8568815 [details]
MozReview Request: bz://1134395/ahal
Attachment #8568815 - Attachment is obsolete: true
Attachment #8619517 - Flags: review+
(Assignee)

Comment 9

3 years ago
Created attachment 8619517 [details]
MozReview Request: Bug 1134395 - mozbuild should pass in rootdir to manifestparser to properly calculate test relpaths, r=gps
You need to log in before you can comment on or make changes to this bug.