[discussion] Physically separate third party libraries from their "parent" library

[pjreiniger]

Looking at portions of the code, it can be difficult to ascertain if they are allwpilib originals, or something ported from a third party library. It is especially difficult when they are done in an incosistant manner; libuv, fmtlib, eigen are separated into subfolders, but llvm, json, stackwalker, etc are not.

I like what is being done with "vendoring" these libraries. However, it gets much more complicated when they live in the tree along side pure wpilib files. Tyler's LLVM upgrade PR #3831 illustrates this fact, as he has to grep for a magic string in the files. The license notices are a mess because you need to "glob" or just say "various files". The styleguide excludes have to call out individual files, of which some are missing and some have since been deleted

My proposal, of which I have a proof of concept for, is simply moving this code into a third_party/xxx subfolder within the library. You can keep the include path the same (i.e. wpi/Compiler.h) but just have it live in a physically different location to make it pop. For example:

wpiutil/src/main/native
|- include
   |- wpi/
      |- algorithm.h (etc, whatever is left)
   |- third_party
      |- llvm/wpi/{files}
      |- span/wpi/{files}
      |- json/wpi/{files}
      |- fmtlib/fmt/{files}

With the same thing replicated in src and test. The build file globs will need to be updated, but it isn't that big of a deal from my experiment. The changes would be opaque to the downstream libraries in this repository, and certainly unnoticeable to the people consuming the libraries from maven. Where exactly you put libraryName vs include vs third_party could be left up for discussion, but I think the example illustrates the high level idea.

In the future, you could investigate compiling these separate static libraries, like wpillvm or something like this. By making the core library that everything depends on smaller, (essentially) full rebuilds might become more infrequent, speeding up developer workflows. A quick look at the code makes me thing that some of the downstream libraries like ntcore and hal might be able to break the wpiutil dependency if these tiny libraries existed, assuming that wpinet goes through

[PeterJohnson]

Note that llvm and json have significant differences in many places to their "upstream" parents.

[pjreiniger]

I'm aware. I tried vendoring json specifically, and it is NOT easy un-"header-only" and de-templatize the thing. Tylers PR shows how much work has to be done for llvm too. I tried to do an inplace vendoring to the same version, and it requires a bunch of patching

But regardless, that doesn't negate the fact that is derived from an external source. The original license headers are still there (as they obviously should be). I don't think the fact that they are heavily modified detracts from that fact. Having it physically separated also gives you that half second of pause "Should I be changing this? Is there some other thing I can do", or at the very least "Peter should maybe look at this a little bit harder since it is a change to what the compiler project does"

[PeterJohnson]

I'd be fine with separating the source trees. Compiling them as separate static libs isn't going to work, however, as you'd end up with duplicate statics in a diamond dynamic library situation (e.g. where two dynamic libs each pull in the same static library).