1138544 - Unskip test_mozbuild_reading.py due to SpiderMonkey build failures

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

We are skipping a sub-test in config/tests/test_mozbuild_reading.py because of a failure in SpiderMonkey builds. We need to get this test unskipped, at least on non-SpiderMonkey builds.

Comment 1

[Gregory Szorc [:gps]]

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: Only skip moz.build reading tests if the environment is bad

Bug 1132771 landed with an unconditional skip of a test in
test_mozbuild_reading.py because of an issue only impacting SpiderMonkey
builds.

First, config/tests are executed during SpiderMonkey builds. This is due
to parts of config/ getting pulled into the objdir for SpiderMonkey
builds. This is a bit awkward, but it's how the world works.

The test in question was failing because
MozbuildObject.from_environment() detects the top source and object
directories, not SpiderMonkey directories. As a result, we were looking
for config.status in a directory that didn't exist because SpiderMonkey
builds aren't full builds. Patching MozbuildObject to do the right thing
is out of scope because this behavior is a hornets nest not worth
disturbing.

This patch introduces a new class for config.status resolution failures.
This probably should have been done months ago instead of raising a
generic Exception.

We modify the failing test to catch "incomplete" build environment
failures and convert those to test skips. The test works on normal
builds, but now skips on SpiderMonkey builds. This should be acceptable,
as the test on the main build includes SpiderMonkey's files, so there is
no loss in overall test coverage.

We likely have other tests that fail without a config.status. We should
consider adopting this pattern in more places. We may even want to
consider a decorator for skipping tests if a build environment is
not present.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8571470 [details] [diff] [review]
Only skip moz.build reading tests if the environment is bad

Review of attachment 8571470 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Gregory Szorc [:gps] from comment #2)
> The test in question was failing because
> MozbuildObject.from_environment() detects the top source and object
> directories, not SpiderMonkey directories. As a result, we were looking
> for config.status in a directory that didn't exist because SpiderMonkey
> builds aren't full builds. Patching MozbuildObject to do the right thing
> is out of scope because this behavior is a hornets nest not worth
> disturbing.

That is not true. Spidermonkey builds have config.status in their topobjdir. In fact, the test works fine when topobjdir is below topsrcdir, but only fails when it's in a non-child directory. The actual problem is that there is no mozinfo.json file for MozbuildObject to find the topsrcdir from that non-child objdir. Why are we not using config.status for that btw?

Comment 4

[Gregory Szorc [:gps]]

The commit message was a bit misleading. I'll rewrite it.

I think the reason we're searching for mozinfo.json instead of config.status is historical reasons. As I mentioned in the commit message, I'd rather not revisit this decision as part of this bug because it may break mach for certain scenarios. I'd encourage you to take another look at the patch.

Comment 5

[Mike Hommey [:glandium]]

Considering how many things are looking at mozinfo.json, I'd rather have js standalone builds emitting one. It's better to do this sooner rather than later because this is bound to bite us again in the future.

Comment 6

[Gregory Szorc [:gps]]

I'm not actively working on this.