search

revery-ui / revery

:zap: Native, high-performance, cross-platform desktop apps - built with Reason!
https://www.outrunlabs.com/revery/
MIT License
8.06k stars 196 forks source link

fix(dependency): esy-skia -> 91c98f6 #1033

Closed glennsl closed 3 years ago

glennsl commented 3 years ago

This brings in a fix to esy-skia that allows it to build with clang 10.2 on Arch Linux.

For some possibly unrelated reason I'm not able to build revery directly due to a failure building ocamlfind, and despite being able to build Oni2 with this same fix, but I'm submitting this now anyway to see what the CI says.

glennsl commented 3 years ago

Ok, looks like there's an issue with OCaml 4.10.2 on my machine, and that Oni2 builds fine because it's pinned to 4.10.0 while the constraint on revery is ~4.10, which then picked 4.10.2 when I touched other dependencies.

I'll continue to investigate this separately and see no reason for that to block this from being merged.

bryphe commented 3 years ago

This brings in a fix to esy-skia that allows it to build with clang 10.2 on Arch Linux.

Nice, thanks @glennsl !

Ok, looks like there's an issue with OCaml 4.10.2 on my machine, and that Oni2 builds fine because it's pinned to 4.10.0 while the constraint on revery is ~4.10, which then picked 4.10.2 when I touched other dependencies.

I believe this is due to this bug: https://github.com/esy/esy/issues/1171 (it was the reason I pinned Onivim to 4.10.0)

esy@0.6.7 may fix this, as it includes a fix for that particular issue - I haven't tested it yet though.

bryphe commented 3 years ago

Testing esy@0.6.7 here: https://github.com/revery-ui/revery/pull/1035

glennsl commented 3 years ago

I believe this is due to this bug: esy/esy#1171 (it was the reason I pinned Onivim to 4.10.0)

Ah, yeah that seems likely.

esy@0.6.7 may fix this, as it includes a fix for that particular issue

I was already using 0.6.7. @esy-nightly/esy fixed it though, but now esy-harfbuzz fails to build...

error: command failed: './esy/prep.sh' (exited with 127)

None of this affects this PR though, so I'll merge it.