search

matprec / rust-font-loader

A font loading utility written in rust.
MIT License
51 stars 21 forks source link

Upgrade core foundation #20

Closed faern closed 6 years ago

faern commented 6 years ago

This is a PR in a series of PRs originating at https://github.com/servo/core-foundation-rs/pull/132

The plan is to make a breaking change to core-foundation and release it as 0.5.0. Hopefully we can manage to bring Servo and it's entire dependency tree up to date as rapidly as possible in combination with this, as to use only the new core-foundation in Servo and all its deps. :)

TODO before this PR can be merged and a new version published:

matprec commented 6 years ago

Please clarify, by referring to "this PR" you refer not to actually this PR but to https://github.com/servo/core-foundation-rs/pull/132, hence merging here is not blocked? The (to this crate) unrelated TODO confuses me.

If so, this PR will be merged as soon as travis is green (Apple environments are rare and often have high backlog).

faern commented 6 years ago

@MSleepyPanda No the TODO is for this PR (#20) This can't be merged as is, as it has a temporary commit changing the paths of the dependencies to point to my github feature branches.

faern commented 6 years ago

I have realized this crate is not actually a dependency of servo. It got caught up in the stream of repos I was upgrading. The reason is that Servo depend on webrender. And in the same repo as webrender is the wrench crate which does depend on this crate. However, neither servo, nor webrender actually depend on wrench, this is the part I missed.

But if you want to upgrade to a newer, simplified core-foundation you can merge this anyway. after the PRs https://github.com/servo/core-foundation-rs/pull/132 and https://github.com/servo/core-text-rs/pull/75 have been merged and published and I have removed the temporary commit d184d0622de71aced4ec93501649f5906bcb90f1

faern commented 6 years ago

@MSleepyPanda This is a bit messy. But if you are interested in why I pre-prepare a ton of PRs then please read the thread in https://github.com/servo/core-foundation-rs/pull/132 that explains that all crates from servo and down the dependency chain should be ready for the upgrade before the actual core-foundation crate is allowed to be merged and published.

matprec commented 6 years ago

Thanks for the clarification! The PR messages were similar what confused me at first, but makes sense.

I have realized this crate is not actually a dependency of servo. It got caught up in the stream of repos I was upgrading. The reason is that Servo depend on webrender. And in the same repo as webrender is the wrench crate which does depend on this crate. However, neither servo, nor webrender actually depend on wrench

Seems about right, but haven't looked into webrender for a while.

Ping me again when the original PR is ready, appreciate your efforts. But it's late here, so the merge might get delayed.

jdm commented 6 years ago

This can be updated to a mergeable state now.

faern commented 6 years ago

Ready for review and merge.

faern commented 6 years ago

Ping @MSleepyPanda, This is now ready for merge. My editor did run rustfmt on parts of the code and I missed that before commiting. So this PR changes a few more lines than needed to upgrade core-foundation. Hope that is fine, otherwise I can remove the few formatting only changes.

matprec commented 6 years ago

LGTM! In total -3 unsafes 🎉

I don't have a mac device on hand, so i have to trust travis here.

Thank you very much!