You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/06/19 17:35:48 UTC
Re: Review Request 67632: Always define PICOJSON_USE_INT64.
> On June 18, 2018, 1:23 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/Makefile.am
> > Lines 96-99 (original), 96-97 (patched)
> > <https://reviews.apache.org/r/67632/diff/1/?file=2042002#file2042002line96>
> >
> > We should probably remove this from CMake too, yeah?
> >
> > ```
> > 3rdparty/CMakeLists.txt
> > 424: PICOJSON_USE_INT64
> > ```
>
> Andrew Schwartzmeyer wrote:
> That said, this part:
>
> > Previously, we relied on the Mesos build system to set
> that macro from the command line, but since we don't
> generate a separate pkg-config file for stout there is
> no way to convey this information to other users of stout.
>
> Isn't true for CMake since it has a notion of interfaces. But still, if we're defining it in the file itself, it's unneeded.
>
> Benjamin Bannier wrote:
> mesos-split, https://reviews.apache.org/r/67634/ :|
>
> Benno Evers wrote:
> As Benjamin said, the issue itself is already fixed in a subsequent review due to `mesos-split`, so I'm dropping this.
>
> However, I think the `cmake` build actually has exactly the same problem, no? Once stout is installed on the system, a program trying to use the stout library has no way anymore to read the cmake interface defintion that was defined in the mesos build system. (or rather, the problem will occur once the cmake build gains an `install` target)
Gotcha.
Though I don't think we'll ever be installing stout, as it's just headers + a lot of third party dependencies, it's still easier to define the flag in the source. Only problem is if _somebody else_ in Mesos starts to use PicoJSON but not through the stout header, but that's perhaps unlikely.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67632/#review204946
-----------------------------------------------------------
On June 18, 2018, 8:53 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67632/
> -----------------------------------------------------------
>
> (Updated June 18, 2018, 8:53 a.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The macro PICOJSON_USE_INT64 must always be defined
> when attempting to use stout's JSON facilities, since
> they unconditionally depend on these features.
>
> Previously, we relied on the Mesos build system to set
> that macro from the command line, but since we don't
> generate a separate pkg-config file for stout there is
> no way to convey this information to other users of stout.
>
> Even for users trying to use stout as part of a libmesos
> installation, it might be surprising that it is required to
> read CPPFLAGS if only the header-only stout part is to be
> used.
>
>
> Diffs
> -----
>
> 3rdparty/stout/Makefile.am 5b922af6cd58dc03c44aacf88637e89f6e289702
> 3rdparty/stout/include/stout/json.hpp 5e738cf6f72f32b852beb61a16787e48082dab14
>
>
> Diff: https://reviews.apache.org/r/67632/diff/2/
>
>
> Testing
> -------
>
> Installed stout on my system and compiled an external program using stout's JSON processing functions.
>
>
> Thanks,
>
> Benno Evers
>
>