Open lilleyse opened 6 months ago
This is a side effect of using the vcpkg libraries for dependency management. I was mostly thinking in terms of the library binaries and not in terms of the additional headers (and header-only libraries) that form part of the Cesium interface.
I was actually a bit surprised when the PR was merged yesterday because it still had some "fixup" commits that I intended to remove via a rebase --autosquash
run.
I believe this problem is tractable, but probably not something I can fix with a quick commit. I would suggest reverting the merge commit and I will create a new PR in draft mode where I will work on correcting this.
Thanks for the heads up.
Actually, thinking about it a bit more, I don't know that this problem is quite as complex as I was first thinking about it. If someone is building Cesium in the fashion you describe, then the required headers are all still on the system, in whatever location they were built using vcpkg. All that's required is the creation of a set of Cesium<Library>Config.cmake
files which declare the vcpkg header locations as part of the INTERFACE
of the library.
So assuming that the downstream users have their ExternalProject_Add
logic followed by a find_package(Cesium...)
, all that would be necessary (after we add this to the Cesium main branch) is to have it documented that they should migrate to a find_package(Cesium... CONFIG)
declaration and this should resolve any missing headers from things like glm and spdlog. The downstream projects will simply pull them from the same location Cesium does... the vcpkg build location.
I will also look into how to make the install mechanism work the same as the prior installer. However, in both cases it's unlikely this will happen today, so again, I understand if you want to revert the merge for now.
The find_package
approach could work. Though the previous behavior is more standard and would be my preference at least.
There's still a bit of time to sort this out before the next release. @kring @azrogers thoughts?
Ok I think I'm going to revert it for now. For Unreal, at least, it'll be essential to copy the headers into the install location, not just reference them from the vcpkg build location. Because we need to actually ship them to end-users of the Cesium for Unreal plugin (who don't build cesium-native or use vcpkg). I think it might take a little while to work this out (if my experience trying to solve the same problem in Conan is any indication), so best to unblock us by reverting while we sort it out. Sorry for the false start @jherico, but we'll get there eventually!
It's reverted. @jherico you should be able to git revert c341a72
and open a new PR with those changes.
@lilleyse can you specify the line you used to configure CMake in your tests? I'm trying to use cmake -B build -DCESIUM_TESTS_ENABLED=OFF -DCMAKE_INSTALL_PREFIX=e:\CesiumInstalled
followed by cmake --build build --target install --config Release
and it's not installing the headers in the destination at all, just the libraries.
It seems to work for me... v0.32.0 on Linux with gcc / make
cmake -B build -DCESIUM_TESTS_ENABLED=OFF -DCMAKE_INSTALL_PREFIX=/home/slilley/Desktop/CesiumNative
cmake --build build --target install --parallel 30
In the new PR I'm breaking up the upstream dependencies into ones where the binary is needed, and ones where the binary and headers are needed (because they're used by the public Cesium headers).
For instance, I noticed that stb
is not exposed in any public header, but also that the headers still get installed in the "before" image of this bug. I'm assuming that this was just a byproduct of the way all upstream dependencies were bundled and not necessarily intentional. However, if any of the upstream packages I've put in "private" really should have their headers installed for whatever reason, let me know.
I can also, of course, just install the entirety of the contents of all the packages, but that feels excessive (though it would actually be less code than working package by package).
The STB case is weird because, while it's not part of the public interface of cesium-native (and therefore we shouldn't need headers), Cesium for Unreal actually also uses STB directly itself. And so that's why cesium-native makes the headers available. This is admittedly super hacky, but we don't have a better solution within easy reach.
Making any of them public is no problem.
On Fri, Mar 1, 2024, 6:09 PM Kevin Ring @.***> wrote:
The STB case is weird because, while it's not part of the public interface of cesium-native (and therefore we shouldn't need headers), Cesium for Unreal actually also uses STB directly itself. And so that's why cesium-native makes the headers available. This is admittedly super hacky, but we don't have a better solution within easy reach.
— Reply to this email directly, view it on GitHub https://github.com/CesiumGS/cesium-native/issues/817#issuecomment-1974196430, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALWNSVKRU53JEJLNRPO6D3YWEYGHAVCNFSM6AAAAABDU5MEKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZUGE4TMNBTGA . You are receiving this because you were mentioned.Message ID: @.***>
First, I'm excited to see that https://github.com/CesiumGS/cesium-native/pull/786 was merged! Thanks @jherico
Though after https://github.com/CesiumGS/cesium-native/pull/786 it seems like third party libraries that are in the public interface are no longer being installed alongside cesium native when running
cmake --build build --target install
. This affects projects that include cesium-native viaExternalProject_Add
.First I wanted to check if this was intentional, and if so, whether there's another approach for including cesium native's dependencies.
Installing Before
Installing After