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)
Firefox Build System
General
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)
7.07 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
52.86 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
1.87 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
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)
Assignee | ||
Comment 2•6 years ago
|
||
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)
Assignee | ||
Comment 3•6 years ago
|
||
These were less straightforward, so I separated them out into a separate patch.
Attachment #8993461 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8993461 -
Attachment is obsolete: true
Attachment #8993507 -
Flags: review?(core-build-config-reviews)
Attachment #8993461 -
Flags: review?(core-build-config-reviews)
Updated•6 years ago
|
Attachment #8993459 -
Flags: review?(core-build-config-reviews) → review+
Comment 7•6 years ago
|
||
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+
Updated•6 years ago
|
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
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e26ad3c03a1 https://hg.mozilla.org/mozilla-central/rev/8e46b3512221 https://hg.mozilla.org/mozilla-central/rev/0d998ecb3661
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 10•6 years ago
|
||
(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.
Description
•