Closed Bug 1477048 Opened 6 years ago Closed 6 years ago

get rid of OS_TEST from moz.build files

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

OS_TEST is this peculiar thing that *looks* like an OS, but is *actually* a CPU
name, which is suboptimal.  We can at least not have it in moz.build files.
Further investigation required to excise it from the build system entirely.
The current code is somewhat non-obvious to a first-time reader, and
OS_TEST is a bizarre thing anyway, since it's actually the name of the
CPU we're running on.  We'd do well to minimize the use of OS_TEST.

Note that the complete nuking of the xptcall/md/unix/moz.build lines are
because we don't support OS X/x86 anymore.
Attachment #8993459 - Flags: review?(core-build-config-reviews)
The deletions in xptcall are when we don't even have support for the CPU
in moz.configure, so I assume that people haven't been compiling on
those architectures for quite some time.
Attachment #8993460 - Flags: review?(core-build-config-reviews)
These were less straightforward, so I separated them out into a separate
patch.
Attachment #8993461 - Flags: review?(core-build-config-reviews)
Blocks: 1476121
I haven't run these through try; going to do that now.  Some of these changes call for a more careful review, because it'd be pretty easily to inadvertently disable something.
Of course I botched things, mostly by using TARGET_CPU when I should have been
using CPU_ARCH.  I'll file a followup for this, we really shouldn't be using
the former.
Attachment #8993460 - Attachment is obsolete: true
Attachment #8993505 - Flags: review?(core-build-config-reviews)
Attachment #8993460 - Flags: review?(core-build-config-reviews)
Attachment #8993461 - Attachment is obsolete: true
Attachment #8993507 - Flags: review?(core-build-config-reviews)
Attachment #8993461 - Flags: review?(core-build-config-reviews)
Attachment #8993459 - Flags: review?(core-build-config-reviews) → review+
Comment on attachment 8993505 [details] [diff] [review]
part 2 - remove non-ipc/chromium moz.build uses of OS_TEST

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

Nice cleanup.

Considering how easy it is to get these tests/compares subtly wrong, I wonder if it would be worth following up to add boolean keys in CONFIG answering basic questions like "is target x86." That way you could just check for key truthiness instead of having to do string compares and membership tests everywhere.

::: xpcom/reflect/xptcall/md/unix/moz.build
@@ -95,5 @@
>              'xptcinvoke_alpha_openbsd.cpp',
>              'xptcstubs_alpha_openbsd.cpp',
>          ]
>  
> -if CONFIG['CPU_ARCH'] == 'arm' or CONFIG['OS_TEST'] == 'sa110':

I had to look up what sa110 was. Good riddance.
Attachment #8993505 - Flags: review?(core-build-config-reviews) → review+
Attachment #8993507 - Flags: review?(core-build-config-reviews) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e26ad3c03a1
part 1 - remove `'86' in CONFIG['OS_TEST']` stanzas from moz.build files; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e46b3512221
part 2 - remove non-ipc/chromium moz.build uses of OS_TEST; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d998ecb3661
part 3 - remove ipc/chromium uses of OS_TEST; r=gps
(In reply to Gregory Szorc [:gps] from comment #7)
> Considering how easy it is to get these tests/compares subtly wrong, I
> wonder if it would be worth following up to add boolean keys in CONFIG
> answering basic questions like "is target x86." That way you could just
> check for key truthiness instead of having to do string compares and
> membership tests everywhere.

FWIW, we do already have `INTEL_ARCHITECTURE` for that specific check (x86 or x86-64):
https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/gfx/2d/moz.build#141

A lot of the weird '86' checks date back to not having normalized values in the target cpu fields, so you'd have to handle it being any of 'i386' or 'i686' or whatever.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: