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

Update ots to v9.1.0 and vendor dependencies #28

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

jschwe
Copy link
Member

@jschwe jschwe commented Feb 4, 2024

Update ots to the latest release and vendor ots and all dependencies.

Motivation for vendoring: Cargo build will recursively clone submodules for git dependencies. ots itself and some of its dependencies have quite large git repos, e.g. ots has 80MB of test font files. Vendoring the sources reduces the required network bandwidth and disk space usage greatly. See also rust-lang/cargo#7987 for more details on the effects on disk usage (not super relevant for us, since we rarely update our dependencies).

To update the vendored dependencies (or double-check the check-in contents match upstream) you can run the script src/deps/update_deps.sh

Ideally, each of the C dependencies would have their own crate, but that can be done later.

@jschwe
Copy link
Member Author

jschwe commented Feb 4, 2024

@mrobinson The diff might look huge, but the actual changes are quite limited:

  • Update src/ots_glue.cc to match ots v9.1.0
  • Move from using git submodules to vendoring source files (recommended approach by ots), since it greatly reduces the size needed to clone (stable cargo currently does not support even shallow cloning)
  • Add a simple bash file src/deps/update_deps.sh, which can update the vendored dependencies. It should be easy to review.
  • Minor adjustments to the CMake to improve the maintainability.
  • Update the readme to also list the licenses of our dependencies, since they will be statically linked into fontsan.

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

This is a really nice change. I especially love the clear documentation.

The only thing I noticed is perhaps we can remove submodules: recursive from the GitHub Actions file at

submodules: recursive
.

Update `ots` to the latest release and vendor ots and all dependencies.
Cargo build will recursively clone submodules for `git` dependencies.
`ots` itself and some of its dependencies have quite large git repos,
e.g. `ots` has 80MB of test font files. Vendoring the sources reduces
the required network bandwidth and disk space usage greatly.

Ideally each of the C dependencies would have their own crate, but that
can be done later.
@jschwe
Copy link
Member Author

jschwe commented Feb 5, 2024

The only thing I noticed is perhaps we can remove submodules: recursive from the GitHub Actions file at

Good point! Removed.

@mrobinson mrobinson added this pull request to the merge queue Feb 5, 2024
Merged via the queue into servo:main with commit 6b859d7 Feb 5, 2024
4 checks passed
@jschwe jschwe deleted the update_ots_cmake branch February 5, 2024 18:23
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.

2 participants