Skip to content

/ servo

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.

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

Allow mach build to explicitly set the media stack #23423

Closed
wants to merge 1 commit into from
Closed

Allow mach build to explicitly set the media stack #23423

wants to merge 1 commit into from

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented May 17, 2019 •
edited by SimonSapin

This allows ./mach build to override the media stack, which is currently hard-wired based on the target.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because it's build infra

This change is Reviewable
@asajeffrey
Allow mach build to explicitly set the media stack
Loading status checks…
3a87a24
@asajeffrey asajeffrey added the A-build label May 17, 2019
@highfive
Copy link

highfive commented May 17, 2019

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/servo/build_commands.py
  • @paulrouget: components/servo/Cargo.toml, ports/glutin/Cargo.toml, components/servo/lib.rs
@asajeffrey
Copy link
Member Author

asajeffrey commented May 17, 2019

The step back is that running raw cargo commands like cargo check will now fail because there's no media-foo feature. The alternative is to use default features, but that has its own pain points, since you can only switch off all default features, there's no way to switch off one default.
@asajeffrey
Copy link
Member Author

asajeffrey commented May 17, 2019

Another option would be to build servo-media-dummy for all platforms, and just only link it on platforms where gstreamer isn't available.
@jdm
jdm reviewed May 17, 2019
View changes
Copy link
Member

jdm left a comment

Another option that would keep the platform logic inside Cargo might be a dummy-media-backend feature, and the cfgs could be any(feature = dummy-media-backend, (os stuff)) and all(not(feature = dummy-media-backend), (os stuff)).
ports/glutin/Cargo.toml
@@ -34,6 +34,8 @@ max_log_level = ["log/release_max_level_info"]
webdriver = ["libservo/webdriver"]
energy-profiling = ["libservo/energy-profiling"]
debugmozjs = ["libservo/debugmozjs"]
media-dummy = ["libservo/servo-media-dummy"]
media-gstreamer = ["libservo/servo-media-gstreamer"]

This comment has been minimized.

Sign in to view
@jdm

jdm May 17, 2019

Member

I suspect we need these in libsimpleservo as well.
@asajeffrey
Copy link
Member Author

asajeffrey commented May 17, 2019

The motivation for this change is so that we have a brute force solution to people who are hitting problems like #21970, which are quite frustrating if you don't need gstreamer.
@afcady
Copy link

afcady commented May 17, 2019

Note that another solution, or at least workaround, for #21970 is just to specify RUSTFLAGS='-L ./support/linux/gstreamer/gst/lib'.
@asajeffrey
Copy link
Member Author

asajeffrey commented May 17, 2019

Yeah, that works for that particular issue, but we keep hitting build problems with gstreamer, it might be nice to have a workaround.
@ferjm
Copy link
Member

ferjm commented May 20, 2019

Thanks @asajeffrey! We definitely need something like this, at least until we have an easier way to setup gstreamer.

I like @jdm's suggestion but I suspect that we'd still have the cargo check issue that you mentioned, right?
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2019

☔ The latest upstream changes (presumably #23451) made this pull request unmergeable. Please resolve the merge conflicts.
@tuncer
Copy link
Contributor

tuncer commented Oct 10, 2019

@asajeffrey, I have a similar use case and would like to see this in master. Would it help if I attempted a fresh rebase?
@asajeffrey
Copy link
Member Author

asajeffrey commented Oct 10, 2019

@tuncer sure!
@tuncer
Copy link
Contributor

tuncer commented Nov 3, 2019

@asajeffrey, sorry for the delay: new diff (patch file). Haven't tried to build yet.
@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 4, 2019

Rather annoyingly I don't think github allows me to make you the owner of this PR :( (https://stackoverflow.com/questions/50781553/how-to-take-ownership-of-a-pull-request has the answer "pretty much nope")

Can you open a new PR with your changes and reference this one? Also cc me in a comment in the new PR (but not the initial commit as that is a recipe for chaos!)
@tuncer
Copy link
Contributor

tuncer commented Nov 4, 2019

I don't mind doing that, but have you considered replacing your local branch with the rebased one and running git push origin +configure-media-stack?
@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 4, 2019

I could, but it means I have to do that for every edit you make, which I think both of us would find kind of frustrating after a while.
@tuncer tuncer mentioned this pull request Nov 5, 2019
Merged
4 of 4 tasks complete
@tuncer
Copy link
Contributor

tuncer commented Nov 5, 2019

@asajeffrey, done, see #24668.
bors-servo added a commit that referenced this pull request Nov 12, 2019
@bors-servo
Auto merge of #24668 - tuncer:configure-media-stack-v2, r=asajeffrey
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
93e737d 
Allow build to explicitly set the media stack

<!-- Please describe your changes on the following line: -->
_[This is based on @asajeffrey's work in #23423, and is a rebase and continuation of that branch]_

This allows `./mach build` to override the media stack, which is currently hard-wired based on the target. To skip gstreamer, run `./mach build -d --media-stack=dummy`.

---
<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because it's build infra

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this pull request Nov 15, 2019
@bors-servo
Auto merge of #24668 - tuncer:configure-media-stack-v2, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f52796a 
Allow build to explicitly set the media stack

<!-- Please describe your changes on the following line: -->
_[This is based on @asajeffrey's work in #23423, and is a rebase and continuation of that branch]_

This allows `./mach build` to override the media stack, which is currently hard-wired based on the target. To skip gstreamer, run `./mach build -d --media-stack=dummy`.

---
<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because it's build infra

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this pull request Nov 16, 2019
@bors-servo
Auto merge of #24668 - tuncer:configure-media-stack-v2, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
501073c 
Allow build to explicitly set the media stack

<!-- Please describe your changes on the following line: -->
_[This is based on @asajeffrey's work in #23423, and is a rebase and continuation of that branch]_

This allows `./mach build` to override the media stack, which is currently hard-wired based on the target. To skip gstreamer, run `./mach build -d --media-stack=dummy`.

---
<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because it's build infra

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this pull request Nov 18, 2019
@bors-servo
Auto merge of #24668 - tuncer:configure-media-stack-v2, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
25a4b20 
Allow build to explicitly set the media stack

<!-- Please describe your changes on the following line: -->
_[This is based on @asajeffrey's work in #23423, and is a rebase and continuation of that branch]_

This allows `./mach build` to override the media stack, which is currently hard-wired based on the target. To skip gstreamer, run `./mach build -d --media-stack=dummy`.

---
<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because it's build infra

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this pull request Nov 21, 2019
@bors-servo
Auto merge of #24668 - tuncer:configure-media-stack-v2, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
46ab840 
Allow build to explicitly set the media stack

<!-- Please describe your changes on the following line: -->
_[This is based on @asajeffrey's work in #23423, and is a rebase and continuation of that branch]_

This allows `./mach build` to override the media stack, which is currently hard-wired based on the target. To skip gstreamer, run `./mach build -d --media-stack=dummy`.

---
<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because it's build infra

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this pull request Nov 22, 2019
@bors-servo
Auto merge of #24668 - tuncer:configure-media-stack-v2, r=asajeffrey
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
10a63cc 
Allow build to explicitly set the media stack

<!-- Please describe your changes on the following line: -->
_[This is based on @asajeffrey's work in #23423, and is a rebase and continuation of that branch]_

This allows `./mach build` to override the media stack, which is currently hard-wired based on the target. To skip gstreamer, run `./mach build -d --media-stack=dummy`.

---
<!-- 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
- [x] These changes fix #23050 (GitHub issue number if applicable)
<!-- Either: -->
- [x] These changes do not require tests because it's build infra

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

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

asajeffrey commented Nov 22, 2019

Replaced by #24668
@asajeffrey asajeffrey closed this Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@ferjm ferjm

Labels
A-build S-awaiting-review S-needs-rebase
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@asajeffrey @highfive @afcady @ferjm @bors-servo @tuncer @jdm
You can’t perform that action at this time.