1330142 - move TARGET_XPCOM_ABI determination to moz.configure

Status: NEW

(Firefox Build System :: General, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move TARGET_XPCOM_ABI to moz.configure

Nothing in js/ uses TARGET_XPCOM_ABI, so the code in js's configure is
dead.  Since TARGET_XPCOM_ABI is a Gecko-only thing, the reasonable
place for it is toolkit/moz.configure.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - remove TARGET_COMPILER_ABI from old-configure

TARGET_COMPILER_ABI is now unused with the removal of TARGET_XPCOM_ABI.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8825557 [details] [diff] [review]
part 1 - move TARGET_XPCOM_ABI to moz.configure

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

::: toolkit/moz.configure
@@ +910,5 @@
>  
>  set_config('ENABLE_MARIONETTE', marionette)
> +
> +@depends(target, c_compiler)
> +def xpcom_abi(target, c_compiler):

I wouldn't expect this to work in a `--disable-compile-environment` build, we probably need to make it conditional.

@@ +917,5 @@
> +        return '%s-msvc' % target.cpu
> +
> +    assert c_compiler.type in ('gcc', 'clang')
> +    # Everything else uses the gcc ABI.
> +    return '%s-gcc3' % ('arm-eabi' if target.cpu == 'arm' else target.cpu,)

In the old-configure implementation, this could end up being "oabi", if that's intentional and no longer relevant, mention it in the commit message.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8825558 [details] [diff] [review]
part 2 - remove TARGET_COMPILER_ABI from old-configure

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

I wouldn't bother splitting this out into a separate patch, I think it makes it harder to see what's going on.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 1 - don't grovel in autoconf.mk for jstest config bits

Nowadays, we dump out most of the interesting results of configure in a
perfectly serviceable Python module; let's use that instead.

Additionally, the jstests framework uses values of TARGET_XPCOM_ABI from
configure as a condition for some of its tests.  But all the conditions
checking the XPCOM ABI only care about the CPU, and checking the XPCOM
ABI is somewhat odd in jstests, which have little to nothing to do with
XPCOM.  Let's switch from using TARGET_XPCOM_ABI to TARGET_CPU, and
changing the test framework and tests accordingly.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 2 - move TARGET_XPCOM_ABI to moz.configure

Nothing in js/ uses TARGET_XPCOM_ABI, so the code in js's configure is
dead.  Since TARGET_XPCOM_ABI is a Gecko-only thing, the reasonable
place for it is toolkit/moz.configure.

TARGET_XPCOM depends on TARGET_COMPILER_ABI, and the value of
TARGET_COMPILER_ABI is easily derivable from the current target and
compiler.  The only notable change we make in the conversion to
moz.configure is that the ARM old-ABI is no longer supported by any of
our current targets: Android and Linux for ARM targets are both
exclusively EABI nowadays, so we have one less case to worry about.
TARGET_COMPILER_ABI's sole purpose was to provide information for
TARGET_XPCOM_ABI, and since we are deriving TARGET_XPCOM_ABI exclusively
inside moz.configure, we can remove TARGET_COMPILER_ABI.

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8911987 [details] [diff] [review]
part 1 - don't grovel in autoconf.mk for jstest config bits

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

Oh, nice! You just happened to be looking at this?

I never knew where this stuff came from before, and was a little disgusted to see it coming from autoconf.mk of all things.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8911988 [details] [diff] [review]
part 2 - move TARGET_XPCOM_ABI to moz.configure

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

::: toolkit/moz.configure
@@ +1199,5 @@
> +    return '%s-gcc3' % ('arm-eabi' if target.cpu == 'arm' else target.cpu,)
> +
> +@depends(xpcom_abi)
> +def xpcom_abi_quoted(abi):
> +    return '"%s"' % abi

It might be a little clearer to just return None here if "abi" == "" because it corresponds more closely to what old-configure used to do.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3914113117f
part 1 - don't grovel in autoconf.mk for jstest config bits; r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4914a29fb26
part 2 - move TARGET_XPCOM_ABI to moz.configure; r=chmanchester

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing jsreftests with "xulRuntime.CPU is undefined":

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eba079bf1a84bea2b77c93f373991e7070f2e4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/43f7bdbadc7d68b32d0e321720a27f9936cd35a9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a4914a29fb2677cb0fa935ca0482ae6a7f4ff556&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134667809&repo=mozilla-inbound

[task 2017-10-03T15:59:52.971Z] 15:59:52     INFO - REFTEST INFO | Application command: /builds/worker/workspace/build/application/firefox/firefox -marionette -profile /tmp/tmp33GU1l.mozrunner
[task 2017-10-03T15:59:53.578Z] 15:59:53     INFO - 1507046393570	Marionette	INFO	Enabled via --marionette
[task 2017-10-03T15:59:55.404Z] 15:59:55     INFO - 1507046395399	Marionette	INFO	Listening on port 2828
[task 2017-10-03T15:59:55.688Z] 15:59:55     INFO - 1507046395682	Marionette	DEBUG	Register listener.js for window 2147483649
[task 2017-10-03T15:59:56.171Z] 15:59:56     INFO - REFTEST INFO | Reading manifest file:///builds/worker/workspace/build/tests/jsreftest/tests/jstests.list
[task 2017-10-03T15:59:56.179Z] 15:59:56     INFO - REFTEST INFO | Dumping JSON representation of sandbox
[task 2017-10-03T15:59:56.182Z] 15:59:56     INFO - REFTEST INFO | {"isDebugBuild":false,"xulRuntime":{"widgetToolkit":"gtk3","OS":"Linux","XPCOMABI":"x86_64-gcc3"},"smallScreen":false,"d2d":false,"dwrite":false,"gpuProcess":false,"azureCairo":false,"azureSkia":true,"skiaContent":true,"azureSkiaGL":0,"contentSameGfxBackendAsCanvas":true,"layersGPUAccelerated":false,"d3d11":false,"d3d9":false,"layersOpenGL":false,"webrender":false,"layersOMTC":true,"advancedLayers":false,"layerChecksEnabled":true,"Android":false,"cocoaWidget":false,"gtkWidget":true,"qtWidget":false,"winWidget":false,"transparentScrollbars":true,"AddressSanitizer":false,"webrtc":true,"stylo":true,"styloVsGecko":false,"skiaPdf":false,"release_or_beta":false,"http":{"userAgent":"Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0","appName":"Mozilla","appVersion":"5.0","platform":"X11","oscpu":"Linux x86_64","misc":"rv:58.0"},"haveTestPlugin":true,"windowsDefaultTheme":false,"nativeThemePref":true,"gpuProcessForceEnabled":false,"prefs":{},"browserIsRemote":true,"asyncPan":true,"usesRepeatResampling":false}
[task 2017-10-03T15:59:56.325Z] 15:59:56    ERROR - REFTEST ERROR | EXCEPTION: TypeError: xulRuntime.CPU is undefined
[task 2017-10-03T15:59:56.327Z] 15:59:56    ERROR - REFTEST ERROR | Got suite_end message before suite_start. Logged with data: {"thread": "None", "extra": {"results": {"AssertionUnexpectedFixed": 0, "Exception": 1, "UnexpectedFail": 0, "UnexpectedPass": 0, "AssertionUnexpected": 0, "AssertionKnown": 0, "Skip": 0, "Random": 0, "Slow": 0, "Pass": 0, "KnownFail": 0, "FailedLoad": 0, "LoadOnly": 0}}, "pid": null, "source": "reftest", "time": 1507046396320, "action": "suite_end"}

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.