Closed Bug 1219530 Opened 4 years ago Closed 4 years ago

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


(Firefox Build System :: General, defect)

Not set


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

Tracking Status
firefox44 --- affected
firefox45 --- fixed
b2g-v2.5 --- fixed


(Reporter: kinetik, Assigned: rillian)




(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
Attachment #8680424 - Flags: review?(mh+mozilla)
Comment on attachment 8680424 [details] [diff] [review]

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 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/ 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]

Review of attachment 8682079 [details] [diff] [review]:

@@ +4030,5 @@
>  fi
> +# Propagate feature switches for code written in rust from
> +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 - apparently, curiously enough, that builds on OS X debug, but every other flavor of every platform says things like
Attachment #8682303 - Flags: review?(mh+mozilla) → review+
Gah. Thanks for the fix.

I guess android libstdc++ is still pretty old.
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.