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.

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

Move surfman out of webxr-api #90

Closed
wants to merge 1 commit into from
Closed

Move surfman out of webxr-api #90

wants to merge 1 commit into from

Conversation

@jdm
Copy link
Member

jdm commented Nov 14, 2019

Having surfman in the public api of webxr-api means that we rebuild some of the largest parts of Servo when changing surfman. These changes use generics to remove that dependency. It's not a perfect end result; it would be conceptually cleaner to push the Surface/SwapChains/SwapChain implementations into Servo itself, but that quickly runs into more complicated type shenanigans without actually yielding a significantly better compilation path.

@jdm jdm force-pushed the private-surfman branch 2 times, most recently from 381f5c4 to f49c45a Nov 14, 2019
@jdm jdm force-pushed the private-surfman branch from f49c45a to c2a2061 Nov 14, 2019
@jdm
Copy link
Member Author

jdm commented Nov 14, 2019

You can see the Servo changes in servo/servo@master...jdm:less-rebuild.

@asajeffrey
Copy link
Member

asajeffrey commented Nov 15, 2019

My attempt at doing this, which is very similar but avoids some issues with the orphan rule by putting the API in surfman-chains-api... https://github.com/asajeffrey/webxr/tree/split-surfman-chains-api-and-impl and https://github.com/asajeffrey/surfman-chains/tree/split-api-and-impl

@asajeffrey
Copy link
Member

asajeffrey commented Nov 15, 2019

I've not done the matching changes to servo yet, but they should be pretty straightforward.

@asajeffrey
Copy link
Member

asajeffrey commented Nov 15, 2019

The matching servo changes which are a lot like yours! https://github.com/asajeffrey/servo/tree/split-surfman-chains-api-and-impl

Copy link
Member

Manishearth left a comment

I'm okay with both ways of doing it.

Note that if we take Alan's approach, you will have to only patch on surfman-chains and not surfman-chains-api when using patch, and edit surfman-chains/Cargo.toml to not have a path dependency, otherwise it will end up clobbering a lot of crates.

@asajeffrey
Copy link
Member

asajeffrey commented Nov 15, 2019

Submitted #91.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 15, 2019

The latest upstream changes (presumably #91) made this pull request unmergeable. Please resolve the merge conflicts.

@asajeffrey
Copy link
Member

asajeffrey commented Nov 15, 2019

Closing in favour of #91. Me me me it's my code by me!

@jdm jdm closed this Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.