Use cargo package layout for mp4parse

RESOLVED FIXED in Firefox 50

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

(Blocks: 1 bug)

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Nathan asked about building the in-tree mp4parse code with cargo. This bug is about importing Cargo.toml and switching to the standard cargo source package layout for the in-tree copy to make this easier. No actual build changes are intended at this point.
Created attachment 8768118 [details]
Bug 1284589 - Move mp4parse source to match upstream.

Make Cargo.toml, build.rs and standard cargo source package
layout available in-tree to facilitate testing cargo-driven
builds of rust code.

Update the moz.build script to build using plain rustc as
before, but referencing the new source location.

Review commit: https://reviewboard.mozilla.org/r/62432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62432/
Attachment #8768118 - Flags: review?(kinetik)
Attachment #8768119 - Flags: review?(kinetik)
Created attachment 8768119 [details]
Bug 1284589 - Result of running the update script.

Implement the changes by running the new update script.

Review commit: https://reviewboard.mozilla.org/r/62434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62434/
Note this still relies on a checked-in copy of the generated mp4parse.h C api header. We'd like cargo to generate this eventually of course. Building `term` and `syntex_syntax` for rusty-cheddar is going to add a few minutes to the clobber build time.

I left byteorder as a module, since making `extern crate` work is still to-do.
The second patch appears to completely remove the sources, and doesn't update moz.build to reflection that removal.  Was that intended?  AFAIK, we want the Rust sources checked into the tree all the time...
Flags: needinfo?(giles)
Oops, forgot to add the files at the new locations. Thanks!
Flags: needinfo?(giles)
Comment on attachment 8768119 [details]
Bug 1284589 - Result of running the update script.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62434/diff/1-2/
Comment on attachment 8768118 [details]
Bug 1284589 - Move mp4parse source to match upstream.

https://reviewboard.mozilla.org/r/62432/#review59258
Attachment #8768118 - Flags: review?(kinetik) → review+
Comment on attachment 8768119 [details]
Bug 1284589 - Result of running the update script.

https://reviewboard.mozilla.org/r/62434/#review59260
Attachment #8768119 - Flags: review?(kinetik) → review+
(In reply to Ralph Giles (:rillian) needinfo me from comment #3)
> Note this still relies on a checked-in copy of the generated mp4parse.h C
> api header. We'd like cargo to generate this eventually of course. Building
> `term` and `syntex_syntax` for rusty-cheddar is going to add a few minutes
> to the clobber build time.

We can also avoid that by switching to rust-bindgen without too much pain (just a bit of grunt work) if that work is already happening (or done) for Stylo.
(In reply to Matthew Gregan [:kinetik] from comment #9)

> We can also avoid that by switching to rust-bindgen without too much pain
> (just a bit of grunt work) if that work is already happening (or done) for
> Stylo.

Right. Small matter of adding libclang as a build dependency. :)

Comment 11

a year ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03ae19f02e2a
Move mp4parse source to match upstream. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/452635f51360
Result of running the update script. r=kinetik
https://hg.mozilla.org/mozilla-central/rev/03ae19f02e2a
https://hg.mozilla.org/mozilla-central/rev/452635f51360
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.