mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 67987: Added rapidjson to the mesos build.
Date Fri, 20 Jul 2018 22:24:37 GMT


> On July 20, 2018, 10:55 a.m., Benno Evers wrote:
> > 3rdparty/Makefile.am
> > Lines 327 (patched)
> > <https://reviews.apache.org/r/67987/diff/1/?file=2061745#file2061745line327>
> >
> >     Shouldn't the header be under `$(RAPIDJSON)/include/rapidjson/rapidjson.h`?
Also, should we maybe add a comment that `rapidjson.h` doesn't include any of the other public
rapidjson headers?

This was another bad copy from picojson (which is single header). I've copied the approach
from elfio now, which puts all headers here.


> 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)

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.


- 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/1/
> 
> 
> Testing
> -------
> 
> Tested at the end of this chain, since this is split across stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message