issues with CMake dependency handling

[milianw]

Hey all,

I'm trying to build OI on arch and run into a lot of trouble.

• the rocksdb version you reference doesn't support libstdc++ from gcc 13, giving compile errors due to missing cstdint includes
• you hardcode LLVM 15, but I have LLVM 16
• you have a mixed bag of system libs, and FetchContent, and git submodules. Why is that? I'm all for system libs, unless you need to have a very specific version of your thirdparty libs - is that the case? If so, would you be opposed to using submodules everywhere? The big advantage there is that we can more easily patch things, and also share the downloaded code across multiple build folders (think debug vs. release)
• your msgpack CMake hackery doesn't work anymore with msgpack-cxx 5.0.0

I have workarounded all of these issues locally now, and would like to clean things up a bit upstream. But for that I would need to know what your constraints are.

My suggestion would be:

• replace FetchContent with git submodules
• then update rocksdb to a newer versions that works with newer compilers.
• alternatively (additionally?) we could use find_package for more dependencies. all of the following are covered by system packages on arch:
  • tomlplusplus
  • glog
  • rocksdb
• support newer msgpack-cxx and only if that's not found fallback to the existing cmake hackery.

Would you be OK with patches going in direction?

[milianw]

OK, LLVM15 is available on my system after all, so updating that isn't that important for now after all. There are some code changes required for LLVM16, like:

@ oi/OICompiler.cpp:242 @ class OIMemoryManager : public RTDyldMemoryManager {
  bool needsToReserveAllocationSpace(void) override {
    return true;
  }
 
  void reserveAllocationSpace(
      uintptr_t, uint32_t, uintptr_t, uint32_t, uintptr_t, uint32_t) override;
      uintptr_t, Align, uintptr_t, Align, uintptr_t, Align) override;

  uint8_t* allocateCodeSection(uintptr_t,
                               unsigned,

and

@ oi/OICompiler.cpp:294 @ void OIMemoryManager::reserveAllocationSpace(uintptr_t codeSize,
  uint64_t totalSz = alignTo((codeSize + roDataSize + rwDataSize), 1024);

  VLOG(1) << "reserveAllocationSpace: codesize " << codeSize << " codeAlign "
          << codeAlign << " roDataSize " << roDataSize << " roDataAlign "
          << roDataAlign << " rwDataSize " << rwDataSize << " rwDataAlign "
          << rwDataAlign << " (Total Size: " << totalSz << ")";
          << codeAlign.value() << " roDataSize " << roDataSize << " roDataAlign "
          << roDataAlign.value() << " rwDataSize " << rwDataSize << " rwDataAlign "
          << rwDataAlign.value() << " (Total Size: " << totalSz << ")";

  Slabs.emplace_back(totalSz, codeSize, roDataSize + rwDataSize + 128);

[JakeHillion]

Thanks for trying! I'll go through the bits you mentioned then say what I think will be the best solutions.

* the rocksdb version you reference doesn't support libstdc++ from gcc 13, giving compile errors due to missing cstdint includes

We can (and should) update this. We run at the head of RocksDB internally, so should always be compatible. I've updated it.

* you hardcode LLVM 15, but I have LLVM 16

Yeah, this is a pain. As you say in the follow up it almost always requires code changes to support a different version of LLVM. It's not impossible though, as https://github.com/iovisor/bpftrace/ manages it. We'll have a chat and see if we can devote a bit more effort to supporting more than one version. The downside is the one we're not using would be poorly tested, but this code rarely breaks so it should be okay. As you say later, a lot of distros package multiple versions of the LLVM libs, but this still isn't ideal.

* you have a mixed bag of system libs, and FetchContent, and  git submodules. Why is that? I'm all for system libs, unless you need to have a very specific version of your thirdparty libs - is that the case? If so, would you be opposed to using submodules everywhere? The big advantage there is that we can more easily patch things, and also share the downloaded code across multiple build folders (think debug vs. release)

I can explain some of the thought process behind this, though I've lost a little with time. We used to have several submodules but switched all but extern/drgn to FetchContent. drgn stayed in extern because we have our own fork for the foreseeable future and used to make changes to it regularly, so the ease of patching was more important than uniformity.

There's also the rocksdb build chaos. Not entirely sure why it invokes CMake separately but trying to swap it to a normal build failed due to flags for me. This should be fixed at some point.

Everything that one can expect to find on all distros has been left as a system dependency.

My view of the way forward here is to add FIND_PACKAGE_ARGS where appropriate to the FetchContent_Declare calls. This lets you use system deps if you have them, and fall back to a download/compile if you don't. It seems like the best of both worlds to me.

@ajor @ttreyer feel free to chime in on this bit.

* your msgpack CMake hackery doesn't work anymore with msgpack-cxx 5.0.0

I think changing msgpack to FetchContent would sort this. We can require a newer version of the system lib and if that fails to be found pull and compile a new version from source.

I have workarounded all of these issues locally now, and would like to clean things up a bit upstream. But for that I would need to know what your constraints are.

My suggestion would be:

* replace FetchContent with git submodules

I think we would still tend toward FetchContent over submodules, mainly due to the better integration with find_package (admittedly we haven't taken advantage of that yet). Again, appreciate the input of @ttreyer and @ajor here.

* then update rocksdb to a newer versions that works with newer compilers.

Done. Thanks for pointing this out! I put it on the latest tagged version which should work.

* alternatively (additionally?) we could use `find_package` for more dependencies. all of the following are covered by system packages on arch:

  * tomlplusplus
  * glog
  * rocksdb

I think adding FIND_PACKAGE_ARGS to tomlplusplus and glog FetchContents will achieve this, while keeping the backup of pulling from the web. rocksdb would be a bit more complicated because the build is awful and needs fixing first.

* support newer msgpack-cxx and only if that's not found fallback to the existing cmake hackery.

Mentioned above.

Would you be OK with patches going in direction?

Yeah! I appreciate the discussion first, but we should definitely move in the direction of easier builds for external contributors. Thanks for your patience and initiative to start working on it. As an aside, I've been working on a Nix flake to make this easier for people to get started with. Including FIND_PACKAGE_ARGS on the FetchContent is a great move for that too.