CesiumGS / cesium-native

Apache License 2.0
414 stars 210 forks source link

Second attempt at migrating packages to vcpkg #820

Closed jherico closed 4 weeks ago

jherico commented 6 months ago

This is an updated version of #786 that attempts to resolve issues with the installation footprint, specifically the lack of installed headers and libraries for the upstream dependencies of Cesium-Native, as covered in #817.

In order to avoid flooding the installation directory with every single library and header that is part of the upstream dependencies for Cesium-Native, this PR breaks down the packages into 4 groups: public, private, and test.

The public packages are any packages where the headers are used in the installed public headers for Cesium, and therefore need to have both the static library and the headers for the package bundled in the install. Things like expected-lite and rapidjson fall into this category.

The private packages are any where the headers are not used in the public interface, so they don't need to be installed, but the static library will still need to be installed for the final link step of the consumer application.

The final category, test, is for libraries that are only used by the test code and therefore don't need any install step at all.

Unfortunately, it looks like Abseil, a dependency of s2geometry, distributes an excessive number of distinct static libraries, and I don't really know how that could be trimmed down easily to just what s2 needs (I'm assuming it's a small subset).

This should much more closely match the current installation layout of the main branch.

jherico commented 3 months ago

@kring feel free to ping me if you're struggling with this. I do have experience with Unreal building as well, so I could try to locally debug some of the problems you're facing.

kring commented 3 months ago

Thanks @jherico, I really appreciate the offer. It's been a pretty painful process getting it working with Unreal, mostly because Unreal / UnrealBuildTool are so... interesting. I haven't hit any hard blocks, but I have hit lots of small ones. My current hurdle is that UnrealBuildTool has special hard-coded logic for libraries named libssl and libcrypto that causes it to wrap them in --whole-archive at link time on Linux. And that in turn causes linker errors about multiply defined symbols.

I can probably fix this just by renaming those libraries. But that's sort of just kicking the can down the road, because in a packaged built, UE's OpenSSL and cesium-native's are going to get slammed together into the same executable, so we'll probably have linker errors at that point with or without --whole-archive.

A perhaps more robust solution is to define an overlay port for OpenSSL that uses Unreal's implementation directly. That'll be very exciting to try to set up I'm sure. 😁

kring commented 3 months ago

I think I have the OpenSSL situation pretty well sorted by using an overlay port that grabs UE's OpenSSL instead of building it from the vcpkg registry.

Something else I could use your advice on though @jherico:

I've set up GitHub Actions vcpkg caching in cesium-unreal, following your lead in cesium-native: https://github.com/CesiumGS/cesium-unreal/blob/f07c2377c775e11c8ac25ecad546e65bcaa7f31b/.github/workflows/buildWindows.yml#L72

This works well and saves a ton of CI time.

But it causes problems when we update one of the several overlay ports that we (unfortunately) need in cesium-unreal. If the previous version is already in the vcpkg directory restored from cache, then vcpkg refuses to install the new version (it says it's already installed). Bumping the version or port-version field in vcpkg.json doesn't seem to help.

The only options that I can see are:

  1. Make sure that the cache key is arranged in such a way that the entire cache is invalidated when an overlay port is updated. This will work, but can cause a fair bit of extra compilation.
  2. Have some kind of complicated system whereby we store hashes of overlay ports with the cache, and explicitly remove any ports (and their dependencies) for which the hash has changed. This would avoid the extra compilation, but would be a good chunk of work to set up.

I vaguely have this notion that vcpkg "manifest mode" could help here, but I don't know if that's actually true.

I'm curious what you think of this, and if you have any advice. Thanks!

jherico commented 3 months ago

I'd probably lean towards option 1 for the time being, essentially adding hashFiles(extern/vcpkg-overlays/**/*) to the cache key, because it's easy to implement, should guarantee correctness and and can be revisited if the overlays end up being frequently updated. I actually haven't worked much with overlays or manifest mode so I can't really speak to that.

kring commented 2 months ago

I just noticed this PR replaces the fillWithRandomBytes implementation with one that isn't cryptographically secure. We'll have to fix that, either by vcpkg-ifying CSPRNG or by switching to an alternative (like OpenSSL, which we already depend upon).

jherico commented 2 months ago

I just noticed this PR replaces the fillWithRandomBytes implementation with one that isn't cryptographically secure. We'll have to fix that, either by vcpkg-ifying CSPRNG or by switching to an alternative (like OpenSSL, which we already depend upon).

I called this out in the original PR comments. CSPRNG isn't a great candidate for vcpkg, since it relies on a deprecated API for the Windows version, and I don't really feel like adopting the project to the extent I would have to in order to update it to the new Windows crypto API.

OTOH, directly using OpenSSL would make it a public facing dependency, instead of a private dependency of s2geometry, so we'd have to install the headers as well.

It's probably easiest to use an #ifdef inside the function to create a Win32 version that uses the current Crypto API and a POSIX version using /dev/urandom for all other cases, which is similar to what CSPRNG does. Will try to get to that today.

kring commented 2 months ago

Thanks @jherico! I meant the comment as a reminder for myself, not to assign you a task, but I really appreciate you tackling it!

OTOH, directly using OpenSSL would make it a public facing dependency, instead of a private dependency of s2geometry, so we'd have to install the headers as well.

Are you sure about that? I don't think we'd need the headers, because OpenSSL wouldn't be in the public API anywhere. It would just be an implementation detail of fillWithRandomBytes. It's possible I'm thinking about it wrong, though.

jherico commented 2 months ago

Are you sure about that?

No, @kring you're right, I wasn't thinking.

kring commented 4 weeks ago

I'm going to merge this into a branch in the main repo. Thanks again @jherico!