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

Refactor crate to use glslang static libraries #1

Merged
merged 12 commits into from
Oct 30, 2020

Conversation

PrototypeNM1
Copy link
Contributor

@PrototypeNM1 PrototypeNM1 commented Sep 7, 2020

Currently waiting for feedback in the Discord to find regressions.

Note: I've only bundled x86_64 libraries for Windows, Mac, and Linux. I've also bundled Android ARM 32 and 64 bit libraries, but no Android AMD64 libraries.

Windows and Android libraries were compiled on my machine for the noted glslang repo tag. Linux and Mac libraries were pulled from the glslang release of the same tag.

If x86 Linux, x86 Windows, or x86/x86_64 Android libraries are desirable I could add those, but would prefer to wait until requested. In particular, the Android 11 x86_64 emulator now work with ARM libraries so I doubt the need is there.

Modified description to match Bevy note.
…ne glslang executable.

Removed dependency on writing spirv shader to a temporary file.

Modified the output of compile(_) to a Vec<u32> instead of a File, otherwise keeps the original API.

Attempts to recreate the original behavior of calling into glslang as faithfully as possible. Use of shader_defs/preamble is simulated by inserting them after `#version` in the provided shader, copying formatting behavior from glslang's source.
Prebuilt glslang libraries and submodule based on glslang tag 8.13.3559.
@PrototypeNM1
Copy link
Contributor Author

I noticed spirv-reflect (another Bevy dependency) vendors and compiles Khronos' spirv-reflect c++ library. If that isn't causing any build issues it may be a better approach than bundling prebuilt static libraries.

@cart
Copy link
Owner

cart commented Sep 9, 2020

If i remember correctly, building glslangvalidator requires some non-standard configuration on windows. It also increases build times by a measurable amount. I think I'd prefer precompiled binaries for now (especially given that this will eventually be replaced by naga).

@cart
Copy link
Owner

cart commented Oct 29, 2020

Macos works as expected. Linux fails with this error:

target/debug/examples/load_gltf: symbol lookup error: /usr/lib/libglslang.so: undefined symbol: _ZNK3spv14SpvBuildLogger14getAllMessagesB5cxx11Ev

@cart
Copy link
Owner

cart commented Oct 29, 2020

But actually only with the "fast compiles" config:

[target.x86_64-unknown-linux-gnu]
linker = "/usr/bin/clang"
rustflags = ["-Clink-arg=-fuse-ld=lld", "-Zshare-generics=y"]

The default configuration works fine.

@enfipy
Copy link

enfipy commented Oct 29, 2020

Maybe it's rustflags or linker somehow blocks link with libs? I tested on Windows and macOS (without config) - works well.

@cart
Copy link
Owner

cart commented Oct 30, 2020

Mac and windows both work with and without "fast compiles" config. Which is interesting because windows also uses lld

@cart
Copy link
Owner

cart commented Oct 30, 2020

Ok narrowed it down a bit more. linux+lld works if I remove /usr/lib/libglslang.so. This probably means that /usr/lib/X.so is taking precedence over the local binaries.

This might also mean that other platforms are broken in this case too and we just haven't encountered it because libglslang wasn't installed as a shared library.

I don't think the solution to this problem should be "delete libglslang.so". Thats installed automatically for a number of packages (ex: vlc and mpv).

I think we need to sort out a way to make sure we use the local libs instead of the shared libs.

@PrototypeNM1
Copy link
Contributor Author

Easy peasy. Added a "namespace" to the library names.

@cart
Copy link
Owner

cart commented Oct 30, 2020

yup that did it!

@cart cart merged commit d3546e3 into cart:master Oct 30, 2020
@cart
Copy link
Owner

cart commented Oct 30, 2020

Weellll we have a problem. Max upload size for crates.io is 10.48MB (compressed). We're at 38MB (compressed) right now.

My immediate hack-ey solution to this problem is to publish multiple crates for each architecture. But this only sort of works:

  1. Windows/Mac/Linux combined are just slightly under the limit at 10.3 MB (which im hoping will go through)
  2. A combined android-only crate is 27.6 MB. Each target is 13.8 MB compressed. So even if we publish two separate crates, we're still over the limit 😢

@cart
Copy link
Owner

cart commented Oct 30, 2020

I sent an email to the crates.io team to see if we can get this limit increased (it sounds like they do have per-crate configurable limits)

If we can't, then we're going to need to get creative with the android deps.

@enfipy
Copy link

enfipy commented Oct 30, 2020

Also, I completely forgot to mention but I think we forgot about "i686-linux-android and x86_64-linux-android targets. @PrototypeNM1 Do you know how to build them?

@enfipy
Copy link

enfipy commented Oct 30, 2020

@cart I have an idea! We can publish a Github Release with libraries (or put them in other Github repo and link it as a submodule) and then download them at the build stage (with Github release we will able to download only that target that we need). It will make the crate small enough and crates.io will accept it.

I will try to make a PoC and PR as I will manage some tasks from my job.

@cart
Copy link
Owner

cart commented Oct 30, 2020

That seems like a reasonable workaround (and would let users only download the required artifacts for a given target), although talking to the internet during build does sketch me out a little bit so I'm hoping we can just do the bundle.

If we go the "internet" route, as much as possible can we download official releases? Looks like android doesnt have official builds, but we should be able to use the Win/Linux/Mac builds from here: https://github.com/KhronosGroup/glslang/releases

@PrototypeNM1
Copy link
Contributor Author

PrototypeNM1 commented Oct 30, 2020

The last stable release from Khronos also lacked Windows builds. I don't think the cost/benefit favors the additional work to download from the official Khronos release page. This is a stopgap until Naga (maybe rust-gpu?) support becomes practical and IMO it's important to guard against over-thinking and over-investing into it.

In that light i686-linux-android and x86_64-linux-android targets are intentionally omitted.

Best to wait to hear back from the crates.io team and proceed from there.

@cart
Copy link
Owner

cart commented Oct 31, 2020

I haven't heard back from the crates.io team. I tried publishing again to see if they just silently bumped the limit, but no dice. We should start thinking about downloading the libraries in build.rs (or other alternatives).

@cart
Copy link
Owner

cart commented Oct 31, 2020

Maybe we could use the bundled libs for desktop and only download the android libs? I'd like to limit the amount of "network access" going on here.

@enfipy
Copy link

enfipy commented Oct 31, 2020

@cart Maybe we can just use github link in Cargo for now? It will just work...

@cart
Copy link
Owner

cart commented Nov 1, 2020

Sadly you can't publish crates with github links

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.

3 participants