You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2018/07/20 03:38:53 UTC
Review Request 67987: Added rapidjson to the mesos build.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/
-----------------------------------------------------------
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
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benno Evers <be...@mesosphere.com>.
> On July 24, 2018, 5:41 p.m., Alexander Rukletsov wrote:
> > 3rdparty/Makefile.am
> > Lines 327 (patched)
> > <https://reviews.apache.org/r/67987/diff/3/?file=2063102#file2063102line327>
> >
> > s/radidjson/rapidjson/
> >
> > Also, I don't see you using `rapidjsondir`, can you please explain me why you declare it?
It's how automake works, the key quote from the manual is:
> A given prefix (e.g. `zar`) is valid if a variable of the same name with
> `dir` appended is defined (e.g. `zardir`).
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206400
-----------------------------------------------------------
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/3/
>
>
> Testing
> -------
>
> Tested at the end of this chain, since this is split across stout/libprocess/mesos.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On July 24, 2018, 5:41 p.m., Alexander Rukletsov wrote:
> > 3rdparty/CMakeLists.txt
> > Lines 37-43 (original), 37-44 (patched)
> > <https://reviews.apache.org/r/67987/diff/3/?file=2063101#file2063101line37>
> >
> > Can you please sort the list?
>
> Benjamin Mahler wrote:
> Hm.. this one is already sorted?
Nope. I've fixed it in https://github.com/apache/mesos/commit/5eed238cbffc27f21ac667145b9c17640bb35141
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206400
-----------------------------------------------------------
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/3/
>
>
> Testing
> -------
>
> Tested at the end of this chain, since this is split across stout/libprocess/mesos.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206400
-----------------------------------------------------------
Fix it, then Ship it!
Please also add an entry to "docs/configuration/autotools.md"
3rdparty/CMakeLists.txt
Lines 37-43 (original), 37-44 (patched)
<https://reviews.apache.org/r/67987/#comment289338>
Can you please sort the list?
3rdparty/Makefile.am
Lines 129-130 (original), 131-132 (patched)
<https://reviews.apache.org/r/67987/#comment289340>
I hate to say it again, but my eyes bleed when I see this.
3rdparty/Makefile.am
Lines 327 (patched)
<https://reviews.apache.org/r/67987/#comment289341>
s/radidjson/rapidjson/
Also, I don't see you using `rapidjsondir`, can you please explain me why you declare it?
- Alexander Rukletsov
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/3/
>
>
> Testing
> -------
>
> Tested at the end of this chain, since this is split across stout/libprocess/mesos.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benjamin Mahler <bm...@apache.org>.
> 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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benjamin Mahler <bm...@apache.org>.
> 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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benno Evers <be...@mesosphere.com>.
> 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.
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.
- Benno
-----------------------------------------------------------
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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206264
-----------------------------------------------------------
3rdparty/Makefile.am
Lines 325 (patched)
<https://reviews.apache.org/r/67987/#comment289181>
Does it? It looks like `jsonify.hpp` actually adds these includes in the next review:
```
#include <rapidjson/stringbuffer.h>
#include <rapidjson/writer.h>
```
3rdparty/Makefile.am
Lines 327 (patched)
<https://reviews.apache.org/r/67987/#comment289160>
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?
3rdparty/Makefile.am
Lines 517 (patched)
<https://reviews.apache.org/r/67987/#comment289162>
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)
3rdparty/rapidjson.md
Lines 11 (patched)
<https://reviews.apache.org/r/67987/#comment289161>
This is not really related to this review, but also came up when discussing the jemalloc patches - if we modify upstreame tarballs, should we maybe gpg-sign the result?
- Benno Evers
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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benjamin Mahler <bm...@apache.org>.
> On July 23, 2018, 9:31 a.m., Benno Evers wrote:
> > 3rdparty/Makefile.am
> > Lines 367 (patched)
> > <https://reviews.apache.org/r/67987/diff/1-2/?file=2061745#file2061745line367>
> >
> > This can be shortened a bit, since we already have the list stored in a variable:
> >
> > ```
> > $(nodist_rapidjson_HEADERS): $(RAPIDJSON)-stamp
> > ```
Ah thanks, looks like elfio should do this too.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206334
-----------------------------------------------------------
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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206334
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/Makefile.am
Lines 367 (patched)
<https://reviews.apache.org/r/67987/#comment289266>
This can be shortened a bit, since we already have the list stored in a variable:
```
$(nodist_rapidjson_HEADERS): $(RAPIDJSON)-stamp
```
- Benno Evers
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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206303
-----------------------------------------------------------
3rdparty/CMakeLists.txt
Lines 444-447 (patched)
<https://reviews.apache.org/r/67987/#comment289208>
Stale comment from copying picojson.
3rdparty/Makefile.am
Lines 514-518 (patched)
<https://reviews.apache.org/r/67987/#comment289210>
Bad copy from picojson, there is no 'make' based install for rapidjson. It uses cmake, but I don't see other cmake install examples here so will have to see what to do (e.g. follow `cp` approach of other libraries here?).
configure.ac
Lines 1854 (patched)
<https://reviews.apache.org/r/67987/#comment289209>
Needs to be rapidjson/rapidjson.h per benno's comments on previous reviews.
- Benjamin Mahler
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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benjamin Mahler <bm...@apache.org>.
> On July 20, 2018, 7:28 a.m., Benjamin Bannier wrote:
> > The patch seems to not contain `3rdparty/rapidjson-1.1.0.tar.gz`. Are you running into https://issues.apache.org/jira/browse/MESOS-3906? If yes, could you try to upload the patch again after applying haosdent's fix?
Ah thanks, any reason not to commit that fix?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206263
-----------------------------------------------------------
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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benjamin Mahler <bm...@apache.org>.
> On July 20, 2018, 7:28 a.m., Benjamin Bannier wrote:
> > The patch seems to not contain `3rdparty/rapidjson-1.1.0.tar.gz`. Are you running into https://issues.apache.org/jira/browse/MESOS-3906? If yes, could you try to upload the patch again after applying haosdent's fix?
>
> Benjamin Mahler wrote:
> Ah thanks, any reason not to commit that fix?
>
> Benjamin Bannier wrote:
> This issue is in `rbtools`, not in our tools. It looks like the issue didn't become clear to them in the patch haosdent posted, and the discussion got derailed and ultimately died.
Ah ok, I'll give that a shot and I commented on that review
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206263
-----------------------------------------------------------
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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On July 20, 2018, 9:28 a.m., Benjamin Bannier wrote:
> > The patch seems to not contain `3rdparty/rapidjson-1.1.0.tar.gz`. Are you running into https://issues.apache.org/jira/browse/MESOS-3906? If yes, could you try to upload the patch again after applying haosdent's fix?
>
> Benjamin Mahler wrote:
> Ah thanks, any reason not to commit that fix?
This issue is in `rbtools`, not in our tools. It looks like the issue didn't become clear to them in the patch haosdent posted, and the discussion got derailed and ultimately died.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206263
-----------------------------------------------------------
On July 20, 2018, 5: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, 5: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
>
>
Re: Review Request 67987: Added rapidjson to the mesos build.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67987/#review206263
-----------------------------------------------------------
The patch seems to not contain `3rdparty/rapidjson-1.1.0.tar.gz`. Are you running into https://issues.apache.org/jira/browse/MESOS-3906? If yes, could you try to upload the patch again after applying haosdent's fix?
- Benjamin Bannier
On July 20, 2018, 5: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, 5: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
>
>