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

Shared object for macos and linux #71

Merged
merged 7 commits into from
Jan 7, 2024
Merged

Conversation

atbrakhi
Copy link
Member

@atbrakhi atbrakhi commented Nov 15, 2023

This PR produces shared objects for macOS and Linux. It was successfully tested on macOS.
See servo/servo#30593

TODO:

  • shared object for angle. .so, .dylib
  • Feature flag

Co-authored-by: Mukilan Thiyagarajan [mukilan@igalia.com]

@atbrakhi
Copy link
Member Author

I'm opening as draft to get some early feedback

@atbrakhi atbrakhi mentioned this pull request Nov 15, 2023
39 tasks
@sagudev
Copy link
Member

sagudev commented Nov 15, 2023

I think this should be behind cargo feature.

CI isn't happy about change, but that might not be PR issue.

@atbrakhi
Copy link
Member Author

atbrakhi commented Nov 15, 2023

ok, so both macos and linux is passing on CI after 2bc03f1. There was an issue with linking libs that was causing test failure in both system.
I was expecting to see some errors on windows! on it now

@atbrakhi atbrakhi changed the title Shared object for macos and linux [WIP] Shared object Nov 29, 2023
@atbrakhi atbrakhi changed the title [WIP] Shared object Shared object for macos and linux Dec 4, 2023
@atbrakhi
Copy link
Member Author

atbrakhi commented Dec 4, 2023

Okay, so now we have everything behind a feature flag. This won't break the current behavior anymore.

I plan to send a separate PR for Windows, as it might take some time for me to get my hands on a Windows system. Meanwhile, we can review this PR.

cc @sagudev

@atbrakhi atbrakhi marked this pull request as ready for review December 4, 2023 12:44
Copy link
Member

@sagudev sagudev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely need CI testing for this

I have yet to do full review.

Copy link
Member

@sagudev sagudev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this PR correctly, it only introduces dylib building but not reusing it right?

build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
@atbrakhi
Copy link
Member Author

atbrakhi commented Dec 4, 2023

If I understand this PR correctly, it only introduces dylib building but not reusing it right?

Indeed, this PR introduces the building of dylib, providing the option to use dylib in other projects if wanted.

By reusing, do you mean within mozangle or in Servo?

@sagudev
Copy link
Member

sagudev commented Dec 4, 2023

By reusing, do you mean within mozangle or in Servo?

Both.

@atbrakhi
Copy link
Member Author

atbrakhi commented Dec 4, 2023

By reusing, do you mean within mozangle or in Servo?

Both.

okey, we are not reusing it anywhere, at least not in this PR.

maybe i am missing something, but specifically for macos and linux, I did not come across a case where we would want to reuse the shared object. I noticed that, It is different for mozjs, where i am having to use shared object within mozjs

@sagudev
Copy link
Member

sagudev commented Dec 4, 2023

On linux and mac we only use translator from angle which is very small, but on windows we actually build the real stuff (dx9 backend) which is big, there we already have build_dlls feature which is needed for GL support in other parts of servo on windows.

I thought the whole point of creating shared objects was to reuse them to have faster compilation on servo. What is point of shared objects if not this (on linux and mac)?

@atbrakhi
Copy link
Member Author

atbrakhi commented Dec 4, 2023

@sagudev i think we are on same page.

so this is how i was planning to go with it:

  • Create shared object for all system (windows is missing in this PR)
  • afterwards reuse them in servo

@atbrakhi
Copy link
Member Author

atbrakhi commented Dec 7, 2023

@sagudev, would you like me to change anything in this PR?

@sagudev
Copy link
Member

sagudev commented Dec 7, 2023

@sagudev, would you like me to change anything in this PR?

It needs rebase because #69 landed, which changes how build.rs works, so I will review it again after rebase, but generally looks good.

@atbrakhi atbrakhi requested a review from sagudev January 4, 2024 21:45
build.rs Show resolved Hide resolved
@sagudev
Copy link
Member

sagudev commented Jan 5, 2024

* Create shared object for all system (windows is missing in this PR)

* afterwards reuse them in servo

I think reusing should be done as part of mozangle (but this is could be done as follow up).

build.rs Show resolved Hide resolved
build.rs Show resolved Hide resolved
sagudev pushed a commit to sagudev/webrender that referenced this pull request Jan 5, 2024
@sagudev sagudev mentioned this pull request Jan 5, 2024
5 tasks
@sagudev
Copy link
Member

sagudev commented Jan 6, 2024

I do not have write access to this repo, so somebody else will need to merge this.

@jdm
Copy link
Member

jdm commented Jan 7, 2024

I do not have write access to this repo, so somebody else will need to merge this.

Are you sure? You're part of the Developers team, which has write access.

@jdm jdm added this pull request to the merge queue Jan 7, 2024
@sagudev
Copy link
Member

sagudev commented Jan 7, 2024

I do not have write access to this repo, so somebody else will need to merge this.

Are you sure? You're part of the Developers team, which has write access.

no access

Merged via the queue into servo:main with commit 671d7e6 Jan 7, 2024
9 checks passed
@msub2 msub2 mentioned this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants