1338655 - mp4parse rust bindings no longer compile with cargo from Rust Nightly 2017-02-10

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Due to a Cargo patch landing that now automatically includes build.rs files regardless of commands in Cargo.toml, the build.rs for mp4parse-capi is included in the build. Since the cheddar crate wasn't vendored in, we can't find it, and get a build error.

Comment 1

[Ralph Giles (:rillian)]

Thanks for the report! Per irc discussion, now that we've vendored syntex, I'll just go ahead and vendor cheddar and stop patching out the build-depends line, assuming that doesn't confuse anything. Fallback is to patch out build.rs, since cargo is apparently auto-detecting it now.

Comment 2

[Ralph Giles (:rillian)]

This seems to be https://github.com/rust-lang/cargo/issues/3391 which suggests we can also patch in `build = false` in Cargo.toml. That would be a better solution if we aren't able to use the generated header.

Going forward, Ted suggested we hack (unmaintained) cheddar to use syn instead of syntex for the build-time speed advantage.

Comment 3

[Ralph Giles (:rillian)]

Attachment: Bug 1338655 - Don't try to build mp4parse bindings.

We use the cheddar crate to generate a C header file
for our mp4parse_capi wrapper crate. Currently we
do this at code check-in time via update-rust.sh.

Cargo 0.18 and later will try to execute a build.rs
file in the crate source tree regardless of whether
it's specified in Cargo.toml so patching out that
line just results in 'crate cheddar not found'.

This change restores the old behaviour by substituting
a 'build = false' line instead.

We do have syntex vendored, but we don't currently build
it by default, so I prefer this solution to just vendoring
cheddar and generating the header at build time. The syntex
crate is quite large and adds significantly to our compile
time.

Review commit: https://reviewboard.mozilla.org/r/111682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111682/

Comment 4

[Ralph Giles (:rillian)]

We're not currently building syntex, so keeping the header generation at import time seems better here.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8836216 [details]
Bug 1338655 - Don't try to build mp4parse bindings.

https://reviewboard.mozilla.org/r/111682/#review113446

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acfa7cfcb32b
Don't try to build mp4parse bindings. r=froydnj

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/acfa7cfcb32b

Comment 9

[Kai Engert (:KaiE:)]

FYI, for me, it doesn't build with build=false, see bug 1340594.

Maybe the change you've landed depends on specific tool versions, and either the build should check for those versions, or somehow that file needs to be parameterized?

Comment 10

[Jorg K (CEST = GMT+2)]

THUNDERBIRD_52_VERBRANCH:
https://hg.mozilla.org/releases/mozilla-esr52/rev/21af4e17cbf9f225e99c4103ac73a72311a702d1

Comment 11

[Jan Beich]

(In reply to Jorg K (GMT+2) from comment #10)
> THUNDERBIRD_52_VERBRANCH:
> https://hg.mozilla.org/releases/mozilla-esr52/rev/
> 21af4e17cbf9f225e99c4103ac73a72311a702d1

It was backed out:
https://hg.mozilla.org/releases/mozilla-esr52/rev/c3c8a7968114