mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 62176: Added cmake dependency check for libsasl2 on non-Windows platforms.
Date Thu, 07 Sep 2017 23:25:06 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62176/#review184908
-----------------------------------------------------------




src/CMakeLists.txt
Lines 615-620 (original), 615-624 (patched)
<https://reviews.apache.org/r/62176/#comment261109>

    I added a note in the other review about `-DHAS_AUTHENTICATION` which we're not checking
here. Rather than check it, I think the whole option should go away.



src/CMakeLists.txt
Lines 615-620 (original), 615-624 (patched)
<https://reviews.apache.org/r/62176/#comment261113>

    As I mentioned in #62105, really all of this code should be contained in `3rdparty/CMakeLists.txt`,
and the only change to this file should be the addition of `sasl2` to the above `target_link_libraries()`
call.



src/CMakeLists.txt
Lines 616-619 (patched)
<https://reviews.apache.org/r/62176/#comment261111>

    This logic I think is fine since there does not exist a `sasl2` CMake module (meaning
we can't use `find_package()`. However, it should live in `3rdparty/CMakeLists.txt` next to
the `sasl2` Windows setup.



src/CMakeLists.txt
Lines 617 (patched)
<https://reviews.apache.org/r/62176/#comment261105>

    I know this is strange, but this can just be `if (SALS2_LIB)`, no string comparison needed.
CMake, for better or worse, treats values like `*-NOTFOUND` as false.



src/CMakeLists.txt
Line 618 (original), 622 (patched)
<https://reviews.apache.org/r/62176/#comment261110>

    This compilation definition should actually be set on the `cyrus_sasl` target as an interface
compilation definition. E.g. `target_compile_definitions(cyrus_sasl PUBLIC LIBSASL_EXPORTS=1)`,
right in `3rdparty/CMakeLists.txt` where the target is created. This ensures locality of configuration/compilation
settings required for dependencies.


- Andrew Schwartzmeyer


On Sept. 7, 2017, 3:21 p.m., John Kordich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62176/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2017, 3:21 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.
> 
> 
> Bugs: MESOS-3110
>     https://issues.apache.org/jira/browse/MESOS-3110
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added cmake dependency check for libsasl2 on non-Windows platforms.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
> 
> 
> Diff: https://reviews.apache.org/r/62176/diff/1/
> 
> 
> Testing
> -------
> 
> I tested this on my Ubuntu 16.04 system. When the libsasl2 library doesn't exist, it
fails the cmake configure/build.  I did not test this on any other platform, but this code
is in a "if (NOT WIN32)" block and won't affect a Windows build.  I'm uncertain if there is
much support for other kinds of builds (like Mac OS), but this should be platform independent.
> 
> 
> Thanks,
> 
> John Kordich
> 
>


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