> On July 20, 2018, 10:55 a.m., Benno Evers wrote: > > 3rdparty/Makefile.am > > Lines 517 (patched) > > <https://reviews.apache.org/r/67987/diff/1/?file=2061745#file2061745line517> > > > > What's the reason that we cannot use the same `DESTDIR=$(INSTALLDIR)` > > pattern as for the other entries here? If I remember correctly, > > `includedir` should default to `$(DESTDIR)/$(PREFIX)/include`, so I guess > > it's to get rid of some default prefix? In this case maybe `PREFIX=` would > > be a little bit more explicit and consistent. (especially since the > > rapidjson build is cmake-based and doesn't support setting `includedir` > > directly) > > Benjamin Mahler wrote: > This was a bad copy; rapidjson doesn't have a makefile. We could run a > cmake install but I noticed it's not done yet for any others? For now I > copied the approach taken for elfio since it uses `cp`. Let me know if that > makes sense, because I'm quite puzzled about the different approaches taken > in this file. > > Benno Evers wrote: > I think you may have confused yourself a little bit here :) Rapidjson > uses cmake, but at the time we want to install it should already be > configured and built, and the generated Makefile does indeed have a working > `install` target. > > But the copying is also fine for a header-only library, and already has > precedence in this file, so I'd say feel free to choose whichever method > feels nicer.
Ah I didn't know that cmake outputs makefiles! I was just looking at the release contents and there wasn't one in there, so it looks like we'd have to run a cmake command to get a makefile in hand? Or directly install from the cmake command? In any case, I think I'll stick to the copying approach since there's precedent and I'm not sure if we require cmake to be present. I left a TODO in the previous update to this patch to explore doing a cmake install instead of copying headers, since the copying approach is pretty brittle to upgrades and is rather verbose when there are a lot of headers. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67987/#review206264 ----------------------------------------------------------- On July 20, 2018, 3:38 a.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67987/ > ----------------------------------------------------------- > > (Updated July 20, 2018, 3:38 a.m.) > > > Review request for mesos, Alexander Rukletsov, Benno Evers, and James Peach. > > > Bugs: MESOS-9092 > https://issues.apache.org/jira/browse/MESOS-9092 > > > Repository: mesos > > > Description > ------- > > This includes a stripped bundle of the latest release. Stripping > is required for licensing (see rapidjson.md), but also helps reduce > the bloat in the mesos git repo. > > Also included is a readme for how to update the dependency. > > > Diffs > ----- > > 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f > 3rdparty/Makefile.am 6123b8a0164d50f0bd771d2e5f485379811e7fcb > 3rdparty/README.md fa9a6483ca2dc4980767a2cb723d654e342e6bb8 > 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a > 3rdparty/rapidjson-1.1.0.tar.gz PRE-CREATION > 3rdparty/rapidjson.md PRE-CREATION > 3rdparty/versions.am 57792800c4b4b67dd55a316ce00480db6c253d34 > configure.ac e86c496140ee9732ad3204ef22ae8ce60cd5c079 > src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 > > > Diff: https://reviews.apache.org/r/67987/diff/2/ > > > Testing > ------- > > Tested at the end of this chain, since this is split across > stout/libprocess/mesos. > > > Thanks, > > Benjamin Mahler > >