Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

script: Disable our dependency in ffmpeg until the media madness is over #12863

Merged
merged 1 commit into from Aug 18, 2016
Merged

script: Disable our dependency in ffmpeg until the media madness is over #12863

merged 1 commit into from Aug 18, 2016

Conversation

emilio
Copy link
Member

@emilio emilio commented Aug 14, 2016 •
edited by larsbergstrom

r? @larsbergstrom

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors.

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/lib.rs, components/script/Cargo.toml, components/script/dom/htmlmediaelement.rs

@highfive
Copy link

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 14, 2016
@emilio
Copy link
Member Author

emilio commented Aug 14, 2016

@bors-servo: try

@bors-servo
Copy link
Contributor

⌛ Trying commit 6f8a85b with merge d4a3ee5...

bors-servo pushed a commit that referenced this pull request Aug 14, 2016
Auto merge of #12863 - emilio:disable-media, r=<try>
d4a3ee5 
script: Disable our dependency in ffmpeg until the media madness is over

<!-- Please describe your changes on the following line: -->

r? @larsbergstrom

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@emilio
Copy link
Member Author

emilio commented Aug 14, 2016

cc @GuillaumeGomez

@GuillaumeGomez
Copy link
Contributor

Wouldn't it better to put it behind a feature disabled by default?

@emilio
Copy link
Member Author

emilio commented Aug 14, 2016

@GuillaumeGomez I agree, but there might be licensing issues with linking ffmpeg, so that's why I removed it.

@GuillaumeGomez
Copy link
Contributor

Any idea how firefox solved these license issues?

@emilio
Copy link
Member Author

emilio commented Aug 14, 2016 •
edited

Not linking it, but dlopening it dinamically: #12687 (comment)

@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 14, 2016
@GuillaumeGomez
Copy link
Contributor

So if we use something along this, we could keep it, right? Wouldn't it be faster to add it in video-metadata-rs crate?

@emilio
Copy link
Member Author

emilio commented Aug 14, 2016

I don't know, maybe? I don't know for sure if linking it dynamically will solve all the problems either, that's a question maybe for @asajeffrey.

@larsbergstrom
Copy link
Contributor

This will also need to disable the corresponding tests.

@GuillaumeGomez Should we do this, or do you think that we will move video-metadata-rs off of ffmpeg in the next day or so? We can certainly land this first to avoid rushing the PRs to that crate, and then turn everything back on later once the dependency is ready again.

@asajeffrey
Copy link
Member

Usual IANAL disclaimer. The FFmpeg licensing page is at: https://www.ffmpeg.org/legal.html

The main requirements are:

  1. Compile FFmpeg without --enable-gpl and without --enable-nonfree.
  2. Use dynamic linking (on windows, this means linking to dlls) for linking with FFmpeg libraries.

There other items in their checklist, but there are the main two.

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 15, 2016
@emilio
Copy link
Member Author

emilio commented Aug 15, 2016

@larsbergstrom: tests updated

@bors-servo: try

@bors-servo
Copy link
Contributor

⌛ Trying commit d200007 with merge b0ce5d2...

bors-servo pushed a commit that referenced this pull request Aug 15, 2016
Auto merge of #12863 - emilio:disable-media, r=<try>
b0ce5d2 
script: Disable our dependency in ffmpeg until the media madness is over

<!-- Please describe your changes on the following line: -->

r? @larsbergstrom

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12863)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 15, 2016
@highfive
Copy link 

  ▶ Unexpected subtest result in /html/semantics/embedded-content/media-elements/event_loadeddata.html:
  └ PASS [expected NOTRUN] setting src attribute on autoplay audio should trigger loadeddata event

  ▶ ERROR [expected TIMEOUT] /html/semantics/embedded-content/media-elements/interfaces/TextTrack/activeCues.html
  └   → TypeError: video.addTextTrack is not a function

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 15, 2016
@emilio
Copy link
Member Author

emilio commented Aug 15, 2016

@bors-servo: try

bors-servo pushed a commit that referenced this pull request Aug 15, 2016
Auto merge of #12863 - emilio:disable-media, r=<try>
9776b70 
script: Disable our dependency in ffmpeg until the media madness is over

<!-- Please describe your changes on the following line: -->

r? @larsbergstrom

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12863)
<!-- Reviewable:end -->
@emilio
Copy link
Member Author

emilio commented Aug 17, 2016 •
edited

@bors-servo: retry

That was my fault, I was investigating WebGL tests failures in the build
slave and I had a wpt socket already up, sorry

On 08/17/2016 02:34 PM, bors-servo wrote:

💔 Test failed - linux-rel
http://build.servo.org/builders/linux-rel/builds/2688

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12863 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABQwuqacmxQ_eNVlkHUDWIWwMd-aHhMWks5qg35ygaJpZM4JkAW5.

bors-servo pushed a commit that referenced this pull request Aug 17, 2016
Auto merge of #12863 - emilio:disable-media, r=larsbergstrom
e994a7e 
script: Disable our dependency in ffmpeg until the media madness is over

<!-- Please describe your changes on the following line: -->

r? @larsbergstrom

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12863)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

⌛ Testing commit b9f8905 with merge e994a7e...

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 17, 2016
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 17, 2016
@highfive
Copy link 

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/perspective-origin-004.htm
  └   → /css-transforms-1_dev/html/perspective-origin-004.htm 68d5e8d6d5edcd2bef5ecaaf62cbbbac863dee22
/css-transforms-1_dev/html/reference/ref-filled-green-100px-square.htm d4aa213e3e31e41d0d8e84aec79e94f860f27178
Testing 68d5e8d6d5edcd2bef5ecaaf62cbbbac863dee22 == d4aa213e3e31e41d0d8e84aec79e94f860f27178

@KiChjang
Copy link
Contributor

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Testing commit b9f8905 with merge fc1f4e9...

bors-servo pushed a commit that referenced this pull request Aug 18, 2016
Auto merge of #12863 - emilio:disable-media, r=larsbergstrom
fc1f4e9 
script: Disable our dependency in ffmpeg until the media madness is over

<!-- Please describe your changes on the following line: -->

r? @larsbergstrom

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12863)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 18, 2016
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 18, 2016
@metajack metajack mentioned this pull request Aug 18, 2016
Closed
11 tasks
@cbrewster
Copy link
Contributor

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Testing commit b9f8905 with merge fb7cb92...

bors-servo pushed a commit that referenced this pull request Aug 18, 2016
Auto merge of #12863 - emilio:disable-media, r=larsbergstrom
fb7cb92 
script: Disable our dependency in ffmpeg until the media madness is over

<!-- Please describe your changes on the following line: -->

r? @larsbergstrom

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12863)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 18, 2016
@bors-servo
Copy link
Contributor

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

@bors-servo bors-servo merged commit b9f8905 into servo:master Aug 18, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 18, 2016
@KiChjang
Copy link
Contributor

FINALLY!

This was referenced Aug 18, 2016
Merged
Merged
@emilio emilio deleted the disable-media branch August 18, 2016 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@larsbergstrom larsbergstrom

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@emilio @highfive @bors-servo @GuillaumeGomez @larsbergstrom @asajeffrey @KiChjang @cbrewster