Closed
Bug 1219530
Opened 5 years ago
Closed 5 years ago
Add a MOZ_RUST conditional #define for the mp4parse-rust library
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox44 affected, firefox45 fixed, b2g-v2.5 fixed)
RESOLVED
FIXED
mozilla45
People
(Reporter: kinetik, Assigned: rillian)
References
Details
Attachments
(2 files, 2 obsolete files)
1.29 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
735 bytes,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
build/autoconf/rust.m4 needs to AC_DEFINE(MOZ_RUST) when --enable-rust succeeds so that existing code calling into Rust code can be built conditionally.
Reporter | ||
Comment 1•5 years ago
|
||
Comment 2•5 years ago
|
||
Comment on attachment 8680424 [details] [diff] [review] bug1219530_v0.patch Review of attachment 8680424 [details] [diff] [review]: ----------------------------------------------------------------- I think a better thing to do is to add ad-hoc defines for each rust library/feature that's going to be used, possibly triggered by confvars.sh variables, or something similar.
Attachment #8680424 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 3•5 years ago
|
||
That makes sense; I'll morph this bug to cover this for our use case.
Summary: #define MOZ_RUST when building with --enable-rust → Add a MOZ_RUST conditional #define for the mp4parse-rust library
Reporter | ||
Updated•5 years ago
|
Attachment #8680424 -
Attachment is obsolete: true
Assignee | ||
Comment 4•5 years ago
|
||
Is this what you had in mind? Define a new MOZ_RUST_MP4PARSE switch in browser/confvars.sh. Check for this in configure, and if rust is enabled and the toolchain is available (MOZ_RUST) set a cpp define with the same name. Don't provide a user-facing configure --enable switch per irc discussion with Ted. FWIW I disagree with this approach for the mp4parse code; we don't intend this to optional code, so gating on the presence of the rust toolchain is sufficient. I'm happy to lead by example if other groups want finer-grained build configuration though.
Attachment #8682079 -
Flags: review?(mh+mozilla)
Reporter | ||
Updated•5 years ago
|
Assignee: kinetik → giles
Comment 5•5 years ago
|
||
Comment on attachment 8682079 [details] [diff] [review] Add MOZ_RUST_MP4PARSE Review of attachment 8682079 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +4030,5 @@ > AC_DEFINE(MOZ_B2GDROID) > fi > > +# Propagate feature switches for code written in rust from confvars.sh > +if test -n "MOZ_RUST" -a -n "MOZ_RUST_MP4PARSE"; then To make it easier for additional things to enter the picture, it might be better to make it two nested ifs.
Attachment #8682079 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 6•5 years ago
|
||
Nest configure conditionals to make it easier to add other rust feature tests. Carrying forward r=glandium. Thanks for the review!
Attachment #8682079 -
Attachment is obsolete: true
Attachment #8682258 -
Flags: review+
Comment 8•5 years ago
|
||
Attachment #8682303 -
Flags: review?(mh+mozilla)
Comment 9•5 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e2bfee574e4e - apparently, curiously enough, that builds on OS X debug, but every other flavor of every platform says things like https://treeherder.mozilla.org/logviewer.html#?job_id=16711985&repo=mozilla-inbound
Updated•5 years ago
|
Attachment #8682303 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 10•5 years ago
|
||
Gah. Thanks for the fix. I guess android libstdc++ is still pretty old.
Comment 12•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/43ce9581fb64
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 13•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/43ce9581fb64
status-b2g-v2.5:
--- → fixed
Updated•3 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•