You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by John Kordich via Review Board <no...@reviews.apache.org> on 2017/09/07 22:21:00 UTC
Review Request 62176: Added cmake dependency check for libsasl2 on
non-Windows platforms.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62176/
-----------------------------------------------------------
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
Re: Review Request 62176: Added cmake dependency check for libsasl2 on
non-Windows platforms.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 62176: Added cmake dependency check for libsasl2 on
non-Windows platforms.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62176/#review184918
-----------------------------------------------------------
FAIL: Mesos tests failed to build. Please check http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62176/logs/mesos-tests-cmake-build.log for any relevant errors
Reviews applied: [62105, 62106, 62176]
Logs available here: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62176/logs
- Mesos Reviewbot Windows
On Sept. 7, 2017, 10: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, 10: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
>
>
Re: Review Request 62176: Added cmake dependency check for libsasl2 on
non-Windows platforms.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Sept. 18, 2017, 12:45 p.m., Andrew Schwartzmeyer wrote:
> > Ship It!
(But wait on required reviews; i.e. this isn't for the whole chain.)
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62176/#review185617
-----------------------------------------------------------
On Sept. 8, 2017, 3:35 p.m., John Kordich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62176/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 3:35 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
> -----
>
> 3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7
>
>
> Diff: https://reviews.apache.org/r/62176/diff/2/
>
>
> 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
>
>
Re: Review Request 62176: Added cmake dependency check for libsasl2 on
non-Windows platforms.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62176/#review185617
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Schwartzmeyer
On Sept. 8, 2017, 3:35 p.m., John Kordich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62176/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 3:35 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
> -----
>
> 3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7
>
>
> Diff: https://reviews.apache.org/r/62176/diff/2/
>
>
> 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
>
>
Re: Review Request 62176: Added cmake dependency check for libsasl2 on
non-Windows platforms.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Sept. 8, 2017, 4:40 p.m., Mesos Reviewbot Windows wrote:
> > FAIL: Mesos tests failed to build. Please check http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62176/logs/mesos-tests-cmake-build.log for any relevant errors
> >
> > Reviews applied: [62105, 62106, 62176]
> >
> > Logs available here: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62176/logs
This fails because the bundle hasn't been committed to our 3rdparty repo yet, which is fine. It just means we need to test manually, and make sure the PR gets merged.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62176/#review185035
-----------------------------------------------------------
On Sept. 8, 2017, 3:35 p.m., John Kordich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62176/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 3:35 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
> -----
>
> 3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7
>
>
> Diff: https://reviews.apache.org/r/62176/diff/2/
>
>
> 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
>
>
Re: Review Request 62176: Added cmake dependency check for libsasl2 on
non-Windows platforms.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62176/#review185035
-----------------------------------------------------------
FAIL: Mesos tests failed to build. Please check http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62176/logs/mesos-tests-cmake-build.log for any relevant errors
Reviews applied: [62105, 62106, 62176]
Logs available here: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62176/logs
- Mesos Reviewbot Windows
On Sept. 8, 2017, 10:35 p.m., John Kordich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62176/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 10:35 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
> -----
>
> 3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7
>
>
> Diff: https://reviews.apache.org/r/62176/diff/2/
>
>
> 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
>
>
Re: Review Request 62176: Added cmake dependency check for libsasl2 on
non-Windows platforms.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62176/
-----------------------------------------------------------
(Updated Sept. 8, 2017, 10:35 p.m.)
Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.
Changes
-------
Addressed suggested comments
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 (updated)
-----
3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7
Diff: https://reviews.apache.org/r/62176/diff/2/
Changes: https://reviews.apache.org/r/62176/diff/1-2/
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