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

Framework includes multiple versions of json.hpp #904

Open
jherico opened this issue Feb 9, 2024 · 3 comments
Open

Framework includes multiple versions of json.hpp #904

jherico opened this issue Feb 9, 2024 · 3 comments
Labels
framework This is relevant to the framework

Comments

@jherico
Copy link
Contributor

jherico commented Feb 9, 2024

Both tinygltf and hwcpipe embed different versions of the nlohmann json.hpp header in their respective source trees, and both of them use the header publicly so downstream users get one or the other depending of the dependency order.

Further, the two versions use different names for their include guards (NLOHMANN_JSON_HPP from tinygltf and INCLUDE_NLOHMANN_JSON_HPP_ from hwcpipe, which is the newer of the two), so it's possible that files end up with both includes.

A short term fix would be to update to a newer version of tinygltf, which would result in both headers using the same include guard and therefore should prevent inclusion of both files into any individual source file. A longer term fix would be to address #893 and ensure the packaging library correctly forces both hwcpipe and tinygltf to depend on the same version of nlohmann, rather than embedding different versions.

@tomadamatkinson
Copy link
Collaborator

An alternative for tinygltf is to define TINYGLTF_NO_INCLUDE_JSON and use the version of nlohmann json that we want. In either case, we likely need to bump our tinygltf version as its 5 years old!

Not sure where HWCPipe includes nlohmann json. Looked through its source tree and couldn't find it. Thats not to say it isn't there!

We are wanting to improve the gltf loader to support more gltf extensions. When we do this, that would be a good time to resolve this issue

@SaschaWillems SaschaWillems changed the title multiple versions of json.hpp Framework includes multiple versions of json.hpp Feb 10, 2024
@SaschaWillems SaschaWillems added the framework This is relevant to the framework label Feb 10, 2024
@SaschaWillems
Copy link
Collaborator

I second that. While tinygltf is indeed outdated, we don't actually use any functionality that would require us an update. On the other hand updating would require us the test all the samples and make sure stuff still works as expected. Just like Tom this should be part of our larger framework rework roadmap.

@jherico
Copy link
Contributor Author

jherico commented Feb 12, 2024

The version of TinyGLTF currently embedded doesn't support TINYGLTF_NO_INCLUDE_JSON, however, I do notice now that it's inclusion of json.hpp only happens inside a #if defined(TINYGLTF_IMPLEMENTATION) block, so the problem is significantly more tractable as its simply a matter of declaring the proper include guard define inside the gltf_loader.cpp right after #define TINYGLTF_IMPLEMENTATION to ensure the outdated JSON header isn't included along with the one from hwcpipe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework This is relevant to the framework
Projects
None yet
Development

No branches or pull requests

3 participants