Closed Bug 1219530 Opened 4 years ago Closed 4 years ago

Add a MOZ_RUST conditional #define for the mp4parse-rust library

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox44 affected, firefox45 fixed, b2g-v2.5 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: kinetik, Assigned: rillian)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch bug1219530_v0.patch (obsolete) — Splinter Review
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attachment #8680424 - Flags: review?(mh+mozilla)
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)
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
Attachment #8680424 - Attachment is obsolete: true
Attached patch Add MOZ_RUST_MP4PARSE (obsolete) — Splinter Review
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)
Assignee: kinetik → giles
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+
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+
Attached patch correctSplinter Review
Attachment #8682303 - Flags: review?(mh+mozilla)
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
Attachment #8682303 - Flags: review?(mh+mozilla) → review+
Gah. Thanks for the fix.

I guess android libstdc++ is still pretty old.
https://hg.mozilla.org/mozilla-central/rev/43ce9581fb64
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.