Update to latest yoga

[nicoburns]

I realise this project isn't really maintained anymore (at least it looks that way), but over at Taffy we're keen on benchmarking against Yoga, and this library would seem to be the obvious way to do it. However, it only seems fair to benchmark against the latest version of yoga. So I thought I'd try to see how hard it was to update (not that hard).

• Upgrades yoga to commit 35f3335efc88efced75aa1db1bb379cf0872831a, which was the latest on the main branch as of 15th December 2022
• Adds new YGPositionStatic variant to YGPosition enum (especially important because it's added to the start of the enum, renumbering the other variants)
• Adds the Guttter type which maps to the new YGGutter
• Adds set_row_gap, get_row_gap, set_column_gap, get_column_gap, set_gap, and get_gap functions to allow setting and getting gutters using (YGNodeStyleSetGap and YGNodeStyleGetGap).
• Upgrades codebase to 2021 edition and fixes cargo warnings.

There are currently 21 tests failing, which I haven't dug into too deeply. They all seem to be to do with absolute position, and they seem to be returning NaN which isn't great.

[bschwind]

@nicoburns wow, very kind of you to update this. It's true this isn't really maintained anymore, but for sure I will still accept PRs! This was one of my earlier Rust projects, and I see I haven't added any github actions CI.

I'll try to add some basic Actions files so we can be more confident in future changes.

[bschwind]

@nicoburns if you prefer, I can fix up formatting and compiler warnings in a separate PR with the addition of github actions. Not sure if that would make this easier or harder for you though, as it'll likely create some merge conflicts.

[bschwind]

Here's the PR for CI, I'll let you decide if you want to merge it before or after your PR

#55

[nicoburns]

@bschwind Ah, amazing. This is a much better response than I was expecting! I think it makes sense to merge your PR first. The first commit of my PR is to upgrade to the 2021 edition of Rust. It may also make sense to apply that as part of your PR. I've rebased my PR on top of yours, so you could cherry pick that commit if you want. Alternatively, I just applied the automated migration, which you could easily do yourself:

• First run the following:

cargo fix --edition
cargo +nightly fmt

• Then manually add edition = "2021" to Cargo.toml

Once that is applied and your PR is merged, you'll find that the changes in my PR are actually pretty minimal. But probably we ought to try to fix the tests before merging it? Computations returning NaN is... not ideal.

[bschwind]

@nicoburns Thanks! I'll go ahead and apply the edition changes and such so that your PR is purely upgrading yoga itself.

Although this crate isn't abandoned, it definitely isn't actively developed like you suspected. The pure-Rust solution in taffy is much preferred.

I still like maintaining the Rust crates I have open sourced though!

[bschwind]

@nicoburns Okay, I've updated the edition, and pushed the changes to master. I'm constantly reminded how nice of a tool cargo is with the painless update :)

[bschwind]

But probably we ought to try to fix the tests before merging it? Computations returning NaN is... not ideal.

Agreed, let's see if it's something easy to fix.

[nicoburns]

Ok, I've found the main issue. Which is that the latest version of yoga is no longer designed to be compiled with --ffast-math. Disabling that flag fixes the majority of the tests. I've also fixed/updated the gentest runner, and regenerated the gentests. We're now only failing 2 tests, both to do with multiline columns.

[nicoburns]

Update: Inspecting the generated tests for C++, it seems that they contain the values that Yoga is actually computing. Presumably Chrome has changed it's behaviour on these tests since yoga last updated their gentests. I'm going to manually adjust these tests to match those in the upstream yoga project.

This is now good to go as far as I'm concerned (modulo the fact that the build is currently failing in CI - I'm not sure why that is, it's working fine for me locally. Possibly a macOS vs Linux difference?)

[nicoburns]

A feature was added to bindgen to address the build issue we're seeing (rust-lang/rust-bindgen#2185). But unfortunately enabling it doesn't seem to fix the issue. I've asked for help here: rust-lang/rust-bindgen#2157, but otherwise I'm afraid I'm at a bit of a loss as how to fix this error. Doesn't help at all that it doesn't reproduce locally.

[bschwind]

@nicoburns do you know of a way on github to approve all future CI runs for you on this PR? I know you're adding things and trying them out, but having to wait for me to approve CI runs probably slows down the experimentation a lot.

[nicoburns]

do you know of a way on github to approve all future CI runs for you on this PR?

I'm not sure. I suspect you'd have to approve my user rather than the PR (because I could of course push anything to the PR if I wanted to). It says "First-time contributors need a maintainer to approve running workflows", so I suspect once I have some code merged into the repo that will fix this.

I'm actually not blocked on this though, because my "fork" (https://github.com/nicoburns/yoga-rs) seems to be independently running the actions and this doesn't seem to require your approval (I guess because the CI is so simple and there are no secrets required, github can make it "just work"?)

[nicoburns]

Ok, something super weird is going on here. I am able to build absolutely fine locally on all of the following:

• macOS (Apple Silicon)
• Ubuntu 22.04 VM (ARM64 VM on Apple Silicon)
• macOS (x86_64)
• Ubuntu 22.04 VM (x86_64 on cloud hosting)

Test procedure

Install pre-requisites:

• Install Rust via rustup
• macOS: Install Xcode and run xcode-select --install to install the command line tools
• Ubuntu: sudo apt update && sudo apt install build-essential libclang-dev

Clone repo

Aside: working with submodules is kinda a pain. I wonder if it would be worth considering vendoring the yoga source instead?

git clone --recursive https://github.com/bschwind/yoga-rs
git remote add nico https://github.com/nicoburns/yoga-rs
git fetch nico
git checkout update-to-latest-yoga
git submodule update --init --recursive --remote
git restore . --recurse-submodules

Run tests

cargo test

Analysis

I think this must have something to do with the CI setup. One would suspect some kind of caching meaning the latest version of the build script isn't getting run. But the CI configuration doesn't seem to be using any caching at all. And I changed the VM image in one commit, which would surely reset any cache.

[nicoburns]

@bschwind

Ok, I got it! If I am understanding things correctly, CI was defaulting to linking against libstdc++ (gcc's C++ stdlib) whereas all my machines were defaulting to libc++ (clang's c++ stdlib) when running bindgen, and bindgen was generating broken bindings for libstdc++. Forcing libc++ with a clang_arg fixes things on CI.

If you approve the latest CI runs then they should now run correctly, and this PR will be passing CI :)

[bschwind]

@nicoburns great work on investigating and solving this! I'll try to review it later today and get it merged. Thanks!

[bschwind]

This looks great! Just one small question.

Thanks for the effort in getting this working on the latest yoga version!

[bschwind]

Thanks for your hard work on this, @nicoburns!