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

[Skia] Import the Skia source code tree #24059

Merged
merged 1 commit into from Feb 8, 2024

Conversation

aperezdc
Copy link
Contributor

@aperezdc aperezdc commented Feb 8, 2024

@aperezdc aperezdc requested a review from a team as a code owner February 8, 2024 10:02
@aperezdc aperezdc self-assigned this Feb 8, 2024
@aperezdc aperezdc added the Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases label Feb 8, 2024
@donny-dont
Copy link
Contributor

The GitHub PR UI is unable to handle any comments I have on this particular patch because of the size of the Skia sources. Could we maybe just land the Skia sources and then add in the build parts in a subsequent patch?

Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

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

I am strongly in favor of this!

@aperezdc
Copy link
Contributor Author

aperezdc commented Feb 8, 2024

The GitHub PR UI is unable to handle any comments I have on this particular patch because of the size of the Skia sources. Could we maybe just land the Skia sources and then add in the build parts in a subsequent patch?

Let's do it that way. I am repurposing this issue to land the Skia sources, and created a new Bugzilla issue for the build system integration.

@aperezdc aperezdc changed the title [CMake] Import the Skia source code and integrate it into the build [Skia] Import the Skia source code tree Feb 8, 2024
@aperezdc aperezdc added the skip-ews Applied to prevent a change from being run on EWS label Feb 8, 2024
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 3086037. Live statuses available at the PR page, #24059

Copy link
Contributor

@donny-dont donny-dont left a comment

Choose a reason for hiding this comment

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

Sony supports the integration of Skia into WebKit as a replacement for Cairo

@aperezdc
Copy link
Contributor Author

aperezdc commented Feb 8, 2024

Done, this now includes only the Skia source and the edit to the linter rules, to avoid false style checker positives for code under Source/ThirdParty/Skia/ πŸƒ

@smfr
Copy link
Contributor

smfr commented Feb 8, 2024

Should we use Source/ThirdParty/skia to match the capitalization of the other directories?

@aperezdc
Copy link
Contributor Author

aperezdc commented Feb 8, 2024

Should we use Source/ThirdParty/skia to match the capitalization of the other directories?

Sure thing, there is no particular reason why we picked the Skia capitalization, so I'll update this to be skia.

@aperezdc
Copy link
Contributor Author

aperezdc commented Feb 8, 2024

Done, now with 25% more lowercase letters in the directory name πŸ˜„

@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for dc1441b. Live statuses available at the PR page, #24059

Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

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

On behalf of the capital letters reduction committee, I (reluctantly) approve this change. (It's too bad this needs to be imported into WebKit's source tree. Would be much nicer as an external dep. Oh well.)

@brentfulgham
Copy link
Contributor

I support adopting Skia as a replacement for Cairo in Windows. This poses no issue to Apple's builds, aside from the performance hit of adding a large number of new files.

Could someone explain why this must be a full import into the tree, rather than an external dependency?

@brentfulgham
Copy link
Contributor

I spoke with Don separately -- it seems like this is similar to ANGLE or BoringSSL where we don't have a stable external versioning system we can trust to have reliable cross-project versioning.

@aperezdc
Copy link
Contributor Author

aperezdc commented Feb 8, 2024

I support adopting Skia as a replacement for Cairo in Windows. This poses no issue to Apple's builds, aside from the performance hit of adding a large number of new files.

πŸ™ŒπŸΌ

Could someone explain why this must be a full import into the tree, rather than an external dependency?

The aim is to provide as less disruption as possible.

During a discussion last week with some of the Skia maintainers they have made it clear to us that it is not in their radar to make guarantees about API/ABI stability, nor making releases of any kind. Therefore, it does not fulfill the packaging requirements for most GNU/Linux distributions, or BSD ports, and will not be available as a system dependency.

Importing the source in the WebKit tree is what makes development most convenient for WebKit developers and does not require any changes in the existing infrastructure (build bots, tooling, scripts, etc.). If we were to use a different distribution mechanism for the Skia sources, we may need changes in a number of places.

Moreover, the practice of shipping such dependencies under Source/ThirdParty/ is not new and has been already in place for quite some time.

Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

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

Looks good.

@aperezdc aperezdc added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 8, 2024
https://bugs.webkit.org/show_bug.cgi?id=268974

Reviewed by Michael Catanzaro, Brent Fulgham and Don Olmstead.

Import the Skia sources at commit 55e8dd4151ce562ce2e61162bf835bef16dd5162.
Follow-up changes will wire building the library and initial support in
the WPE port.

* Source/ThirdParty/skia: Added.
* Tools/Scripts/webkitpy/style/checker.py: Ignore most of the rules
for ThirdParty/skia.

Canonical link: https://commits.webkit.org/274314@main
@webkit-commit-queue
Copy link
Collaborator

Committed 274314@main (a69ccc8): https://commits.webkit.org/274314@main

Reviewed commits have been landed. Closing PR #24059 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit a69ccc8 into WebKit:main Feb 8, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 8, 2024
@aperezdc aperezdc deleted the aperezdc/import-skia branch May 10, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-ews Applied to prevent a change from being run on EWS Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases
Projects
None yet
7 participants