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

LibWeb: allow using system skia #382

Merged
merged 2 commits into from
Jul 5, 2024
Merged

Conversation

fgaz
Copy link
Contributor

@fgaz fgaz commented Jul 3, 2024

unofficial-skia is a vcpkg-specific package. With this change ladybird can be built against skia as provided by system package managers such as guix, mingw, and (soon) nix. All those packages include a .pc file, so we use pkg-config.

I made it check for unofficial-skia first. I can do the opposite if preferred.

Originally written for the downstream NixOS package https://github.com/NixOS/nixpkgs/pull/324050/files#diff-8c404e5e6584a620c2eefd7acfd8951eb8988aeba3cfb07408eed81819c2626a

@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@kalenikaliaksandr
Copy link
Member

kalenikaliaksandr commented Jul 3, 2024

what are the advantages of using system's Skia?

from my experience with Skia so far, API seem to change quite significantly across versions, e.g. our painter should not build with very latest Skia. so I think it's better to always link with the specific version we get from vcpkg.

@fgaz
Copy link
Contributor Author

fgaz commented Jul 3, 2024

Well... the advantage is that I can actually build the project now. vcpkg doesn't work well on NixOS. This will also make it easier for distros to package ladybird, since downloading random stuff at build time like vcpkg does is not allowed in most distros.

If you really need a specific skia version, I can add a version constraint, but it will have to be kept in sync with the one in vcpkg.json

@fgaz
Copy link
Contributor Author

fgaz commented Jul 3, 2024

To be clear, we will need to do something like this to update ladybird in nixpkgs, as you can see from https://github.com/NixOS/nixpkgs/pull/324050/files#diff-8c404e5e6584a620c2eefd7acfd8951eb8988aeba3cfb07408eed81819c2626a. I'm hoping to upstream (a modified version of) that patch

@kalenikaliaksandr
Copy link
Member

It's unfortunate that vcpkg does not work on NixOS yet. For now, let's always use Skia from vcpkg, as it is less likely to cause any build issues on popular Linux distributions and macOS, which are the platforms we care about most at the moment.

@sideeffect42
Copy link
Contributor

sideeffect42 commented Jul 3, 2024

If you need a specific state of Skia, please vendor it like everybody else does.
Requiring vcpkg will prevent Ladybird from appearing in pretty much any Linux package manager.

(or add a fallback which works for distros, like cairo or something.)

@ADKaster
Copy link
Member

ADKaster commented Jul 3, 2024

Hmm. I think we can accept this, as long as we specify a specific version. Keeping the versions in sync isn't that big a deal.

@ADKaster ADKaster reopened this Jul 3, 2024
@kalenikaliaksandr
Copy link
Member

Hmm. I think we can accept this, as long as we specify a specific version. Keeping the versions in sync isn't that big a deal.

I don't think it's only the version that matters, but the large variety of skia'a build configuration options -- I can't confidently say we do not rely on a specific config provided by vcpkg.

unofficial-skia is a vcpkg-specific package. With this change ladybird
can be built against skia as provided by system package managers such as
guix, mingw, and (soon) nix. All those packages include a .pc file, so
we use pkg-config.
@fgaz
Copy link
Contributor Author

fgaz commented Jul 3, 2024

I added the constraint.

Feel free to direct any bug reports from users of the NixOS package to our issue tracker. That's where they should be reporting anyway, unless they verified that the issue is not caused by NixOS. Most distros follow the same policy.

Also note that skia from vcpkg takes priority, so this doesn't affect at all the ladybird.sh build process.

This way we only have to update it in one place.
@fgaz
Copy link
Contributor Author

fgaz commented Jul 3, 2024

In the second commit I solved the duplication problem, at the cost of complexity in the cmake file. I kept it separate so you can choose to merge both or just the first one.

@socksy socksy mentioned this pull request Jul 5, 2024
@qbit
Copy link

qbit commented Jul 5, 2024

FWIW, using vcpkg only would prevent building / packaging on OpenBSD (probably other BSDs as well).

Copy link
Member

@ADKaster ADKaster left a comment

Choose a reason for hiding this comment

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

Thanks, let's go with these. If anyone using PkgConfig to find skia has troubles, we'll point them to their downstream packager.

We'll probably eventually(?) end up vendoring skia, but downstream packagers will want to supply their own version anyway so 🤷

@ADKaster ADKaster merged commit 210e6ed into LadybirdBrowser:master Jul 5, 2024
6 checks passed
@fgaz fgaz deleted the system-skia branch July 5, 2024 19:06
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.

6 participants