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/07/13 21:04:46 UTC
Review Request 67916: Patched Google Test with upstream bugfix.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
Bugs: MESOS-8990
https://issues.apache.org/jira/browse/MESOS-8990
Repository: mesos
Description
-------
Per MESOS-8990, our Google Test dependency needs a patch from
upstream, https://github.com/google/googletest/pull/1620, in order to
continue building with the next version of MSVC (and potentially other
compilers).
This patch file was generated by cherry-picking `f66ab00` from
`master` onto `release-1.8.0` in the Google Test repo, and resolving
the merge conflict.
Diffs
-----
3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
3rdparty/googletest-release-1.8.0.patch PRE-CREATION
Diff: https://reviews.apache.org/r/67916/diff/1/
Testing
-------
Clean build on Windows using CMake (which is the only place this patch currently applies). It is not yet known if the Autotools system will also need this patch. Do we want it added there anyway?
Thanks,
Andrew Schwartzmeyer
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/#review206371
-----------------------------------------------------------
Patch looks great!
Reviews applied: [67916]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On July 13, 2018, 9:04 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> -----------------------------------------------------------
>
> (Updated July 13, 2018, 9:04 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
>
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/googletest-release-1.8.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67916/diff/1/
>
>
> Testing
> -------
>
> Clean build on Windows using CMake (which is the only place this patch currently applies). It is not yet known if the Autotools system will also need this patch. Do we want it added there anyway?
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/#review206614
-----------------------------------------------------------
PASS: Mesos patch 67916 was successfully built and tested.
Reviews applied: `['67916']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2003/mesos-review-67916
- Mesos Reviewbot Windows
On July 30, 2018, 11:50 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> -----------------------------------------------------------
>
> (Updated July 30, 2018, 11:50 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
>
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
>
> Additionally, we need to define `GTEST_LANG_CXX11=1` when including
> the GoogleTest headers such that the patch is used.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4
> 3rdparty/googletest-release-1.8.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67916/diff/2/
>
>
> Testing
> -------
>
> Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes:
>
> ```
> # Set the default standard to C++11 for all targets.
> -set(CMAKE_CXX_STANDARD 11)
> -set(CMAKE_CXX_STANDARD_REQUIRED ON)
> +# set(CMAKE_CXX_STANDARD 11)
> +# set(CMAKE_CXX_STANDARD_REQUIRED ON)
> # Do not use, for example, `-std=gnu++11`.
> -set(CMAKE_CXX_EXTENSIONS OFF)
> +# set(CMAKE_CXX_EXTENSIONS OFF)
> +add_compile_options(/std:c++latest)
> +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
> +add_definitions(-D_HAS_AUTO_PTR_ETC=1)
> +add_definitions(-D_HAS_TR1_NAMESPACE=1)
> +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
> +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
> ```
>
> After adding the necessary compile interface definition, `ninja tests` successfully built on Windows with this patch applied, after not building before the patch.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/#review206676
-----------------------------------------------------------
PASS: Mesos patch 67916 was successfully built and tested.
Reviews applied: `['67916']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2013/mesos-review-67916
- Mesos Reviewbot Windows
On July 31, 2018, 6:02 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> -----------------------------------------------------------
>
> (Updated July 31, 2018, 6:02 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
>
> This patch file was generated with `git format-patch` for the commit
> `f66ab00704cd47e4e63ef6d425ca14b9192aaebb`.
>
> Additionally, we need to define `GTEST_LANG_CXX11=1` when including
> the GoogleTest headers such that the patch is used.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4
> 3rdparty/googletest-release-1.8.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67916/diff/3/
>
>
> Testing
> -------
>
> Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes:
>
> ```
> # Set the default standard to C++11 for all targets.
> -set(CMAKE_CXX_STANDARD 11)
> -set(CMAKE_CXX_STANDARD_REQUIRED ON)
> +# set(CMAKE_CXX_STANDARD 11)
> +# set(CMAKE_CXX_STANDARD_REQUIRED ON)
> # Do not use, for example, `-std=gnu++11`.
> -set(CMAKE_CXX_EXTENSIONS OFF)
> +# set(CMAKE_CXX_EXTENSIONS OFF)
> +add_compile_options(/std:c++latest)
> +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
> +add_definitions(-D_HAS_AUTO_PTR_ETC=1)
> +add_definitions(-D_HAS_TR1_NAMESPACE=1)
> +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
> +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
> ```
>
> After adding the necessary compile interface definition, `ninja tests` successfully built on Windows with this patch applied, after not building before the patch.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/
-----------------------------------------------------------
(Updated July 31, 2018, 11:02 a.m.)
Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
Changes
-------
Used original upstream patch as GNU Patch is capable of applying it, though `git cherry-pick` (my original method) could not.
Bugs: MESOS-8990
https://issues.apache.org/jira/browse/MESOS-8990
Repository: mesos
Description (updated)
-------
Per MESOS-8990, our Google Test dependency needs a patch from
upstream, https://github.com/google/googletest/pull/1620, in order to
continue building with the next version of MSVC (and potentially other
compilers).
This patch file was generated with `git format-patch` for the commit
`f66ab00704cd47e4e63ef6d425ca14b9192aaebb`.
Additionally, we need to define `GTEST_LANG_CXX11=1` when including
the GoogleTest headers such that the patch is used.
Diffs (updated)
-----
3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4
3rdparty/googletest-release-1.8.0.patch PRE-CREATION
Diff: https://reviews.apache.org/r/67916/diff/3/
Changes: https://reviews.apache.org/r/67916/diff/2-3/
Testing
-------
Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes:
```
# Set the default standard to C++11 for all targets.
-set(CMAKE_CXX_STANDARD 11)
-set(CMAKE_CXX_STANDARD_REQUIRED ON)
+# set(CMAKE_CXX_STANDARD 11)
+# set(CMAKE_CXX_STANDARD_REQUIRED ON)
# Do not use, for example, `-std=gnu++11`.
-set(CMAKE_CXX_EXTENSIONS OFF)
+# set(CMAKE_CXX_EXTENSIONS OFF)
+add_compile_options(/std:c++latest)
+add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
+add_definitions(-D_HAS_AUTO_PTR_ETC=1)
+add_definitions(-D_HAS_TR1_NAMESPACE=1)
+add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
+add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
```
After adding the necessary compile interface definition, `ninja tests` successfully built on Windows with this patch applied, after not building before the patch.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On July 31, 2018, 1:27 a.m., Benjamin Bannier wrote:
> > LGTM. I reopened https://reviews.apache.org/r/67916/#comment289263, using the original upstream patch would be great if possible.
Ben, this did use the oiginal upsteam patch. I cloned the repo, checked out the tag we bundle, and cherry-picked the upstream patch from master, then created a patch file from the applied diff. The commit in master does not apply to 1.8.0 perfectly, so I can't just turn it into a patch, it had to be applied and conflicts fixed. I'm closing the issue again, because this is the original upstream patch, or as close as possible as we can get.
If you have another way to do it, I'm all ears!
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/#review206647
-----------------------------------------------------------
On July 30, 2018, 11:50 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> -----------------------------------------------------------
>
> (Updated July 30, 2018, 11:50 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
>
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
>
> Additionally, we need to define `GTEST_LANG_CXX11=1` when including
> the GoogleTest headers such that the patch is used.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4
> 3rdparty/googletest-release-1.8.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67916/diff/2/
>
>
> Testing
> -------
>
> Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes:
>
> ```
> # Set the default standard to C++11 for all targets.
> -set(CMAKE_CXX_STANDARD 11)
> -set(CMAKE_CXX_STANDARD_REQUIRED ON)
> +# set(CMAKE_CXX_STANDARD 11)
> +# set(CMAKE_CXX_STANDARD_REQUIRED ON)
> # Do not use, for example, `-std=gnu++11`.
> -set(CMAKE_CXX_EXTENSIONS OFF)
> +# set(CMAKE_CXX_EXTENSIONS OFF)
> +add_compile_options(/std:c++latest)
> +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
> +add_definitions(-D_HAS_AUTO_PTR_ETC=1)
> +add_definitions(-D_HAS_TR1_NAMESPACE=1)
> +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
> +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
> ```
>
> After adding the necessary compile interface definition, `ninja tests` successfully built on Windows with this patch applied, after not building before the patch.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On July 31, 2018, 10:27 a.m., Benjamin Bannier wrote:
> > LGTM. I reopened https://reviews.apache.org/r/67916/#comment289263, using the original upstream patch would be great if possible.
>
> Andrew Schwartzmeyer wrote:
> Ben, this did use the oiginal upsteam patch. I cloned the repo, checked out the tag we bundle, and cherry-picked the upstream patch from master, then created a patch file from the applied diff. The commit in master does not apply to 1.8.0 perfectly, so I can't just turn it into a patch, it had to be applied and conflicts fixed. I'm closing the issue again, because this is the original upstream patch, or as close as possible as we can get.
>
> If you have another way to do it, I'm all ears!
Sorry for possibly being very dense. I did this
# Applied your patch.
mesos [master?] % ./support/apply-reviews.py -r 67916 -c -n -s
# <SNIP>
# Extracted the _original_ upstream patch.
googletest [master?] % git format-patch f66ab00704cd47e4e63ef6d425ca14b9192aaebb~1..f66ab00704cd47e4e63ef6d425ca14b9192aaebb
0001-Upstream-cl-199129756.patch
# Use the _original_ upstream patch in Mesos.
mesos [master?] % mv ~/src/googletest/0001-Upstream-cl-199129756.patch 3rdparty/googletest-release-1.8.0.patch
# `patch`, at least on macos-10.13.6, is able to automatically apply this patch.
mesos [master??] % ninja -C_ 3rdparty/googletest-1.8.0
# <SNIP>
[1/5] Performing patch step for 'googletest-1.8.0'
patching file googletest/include/gtest/gtest-printers.h
Hunk #1 succeeded at 581 with fuzz 2 (offset -55 lines).
patching file googletest/test/gtest-printers_test.cc
Hunk #1 succeeded at 1111 (offset -4 lines).
# <SNIP>
Does this not work on e.g., Windows?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/#review206647
-----------------------------------------------------------
On July 30, 2018, 8:50 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> -----------------------------------------------------------
>
> (Updated July 30, 2018, 8:50 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
>
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
>
> Additionally, we need to define `GTEST_LANG_CXX11=1` when including
> the GoogleTest headers such that the patch is used.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4
> 3rdparty/googletest-release-1.8.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67916/diff/2/
>
>
> Testing
> -------
>
> Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes:
>
> ```
> # Set the default standard to C++11 for all targets.
> -set(CMAKE_CXX_STANDARD 11)
> -set(CMAKE_CXX_STANDARD_REQUIRED ON)
> +# set(CMAKE_CXX_STANDARD 11)
> +# set(CMAKE_CXX_STANDARD_REQUIRED ON)
> # Do not use, for example, `-std=gnu++11`.
> -set(CMAKE_CXX_EXTENSIONS OFF)
> +# set(CMAKE_CXX_EXTENSIONS OFF)
> +add_compile_options(/std:c++latest)
> +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
> +add_definitions(-D_HAS_AUTO_PTR_ETC=1)
> +add_definitions(-D_HAS_TR1_NAMESPACE=1)
> +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
> +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
> ```
>
> After adding the necessary compile interface definition, `ninja tests` successfully built on Windows with this patch applied, after not building before the patch.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/#review206647
-----------------------------------------------------------
Ship it!
LGTM. I reopened https://reviews.apache.org/r/67916/#comment289263, using the original upstream patch would be great if possible.
- Benjamin Bannier
On July 30, 2018, 8:50 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> -----------------------------------------------------------
>
> (Updated July 30, 2018, 8:50 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
>
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
>
> Additionally, we need to define `GTEST_LANG_CXX11=1` when including
> the GoogleTest headers such that the patch is used.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4
> 3rdparty/googletest-release-1.8.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67916/diff/2/
>
>
> Testing
> -------
>
> Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes:
>
> ```
> # Set the default standard to C++11 for all targets.
> -set(CMAKE_CXX_STANDARD 11)
> -set(CMAKE_CXX_STANDARD_REQUIRED ON)
> +# set(CMAKE_CXX_STANDARD 11)
> +# set(CMAKE_CXX_STANDARD_REQUIRED ON)
> # Do not use, for example, `-std=gnu++11`.
> -set(CMAKE_CXX_EXTENSIONS OFF)
> +# set(CMAKE_CXX_EXTENSIONS OFF)
> +add_compile_options(/std:c++latest)
> +add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
> +add_definitions(-D_HAS_AUTO_PTR_ETC=1)
> +add_definitions(-D_HAS_TR1_NAMESPACE=1)
> +add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
> +add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
> ```
>
> After adding the necessary compile interface definition, `ninja tests` successfully built on Windows with this patch applied, after not building before the patch.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/
-----------------------------------------------------------
(Updated July 30, 2018, 11:50 a.m.)
Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
Changes
-------
Fixed build by adding compile interface definition.
Bugs: MESOS-8990
https://issues.apache.org/jira/browse/MESOS-8990
Repository: mesos
Description (updated)
-------
Per MESOS-8990, our Google Test dependency needs a patch from
upstream, https://github.com/google/googletest/pull/1620, in order to
continue building with the next version of MSVC (and potentially other
compilers).
This patch file was generated by cherry-picking `f66ab00` from
`master` onto `release-1.8.0` in the Google Test repo, and resolving
the merge conflict.
Additionally, we need to define `GTEST_LANG_CXX11=1` when including
the GoogleTest headers such that the patch is used.
Diffs (updated)
-----
3rdparty/CMakeLists.txt d8d113c17d124b763659dcc5ace9066499de6cd4
3rdparty/googletest-release-1.8.0.patch PRE-CREATION
Diff: https://reviews.apache.org/r/67916/diff/2/
Changes: https://reviews.apache.org/r/67916/diff/1-2/
Testing (updated)
-------
Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes:
```
# Set the default standard to C++11 for all targets.
-set(CMAKE_CXX_STANDARD 11)
-set(CMAKE_CXX_STANDARD_REQUIRED ON)
+# set(CMAKE_CXX_STANDARD 11)
+# set(CMAKE_CXX_STANDARD_REQUIRED ON)
# Do not use, for example, `-std=gnu++11`.
-set(CMAKE_CXX_EXTENSIONS OFF)
+# set(CMAKE_CXX_EXTENSIONS OFF)
+add_compile_options(/std:c++latest)
+add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
+add_definitions(-D_HAS_AUTO_PTR_ETC=1)
+add_definitions(-D_HAS_TR1_NAMESPACE=1)
+add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
+add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
```
After adding the necessary compile interface definition, `ninja tests` successfully built on Windows with this patch applied, after not building before the patch.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/
-----------------------------------------------------------
(Updated July 30, 2018, 11:02 a.m.)
Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
Changes
-------
Does not fix MESOS-8990 (yet).
Bugs: MESOS-8990
https://issues.apache.org/jira/browse/MESOS-8990
Repository: mesos
Description
-------
Per MESOS-8990, our Google Test dependency needs a patch from
upstream, https://github.com/google/googletest/pull/1620, in order to
continue building with the next version of MSVC (and potentially other
compilers).
This patch file was generated by cherry-picking `f66ab00` from
`master` onto `release-1.8.0` in the Google Test repo, and resolving
the merge conflict.
Diffs
-----
3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
3rdparty/googletest-release-1.8.0.patch PRE-CREATION
Diff: https://reviews.apache.org/r/67916/diff/1/
Testing (updated)
-------
Repro'ed MESOS-8990 on Windows with Visual Studio Preview, and the following (temporary) CMake changes:
```
# Set the default standard to C++11 for all targets.
-set(CMAKE_CXX_STANDARD 11)
-set(CMAKE_CXX_STANDARD_REQUIRED ON)
+# set(CMAKE_CXX_STANDARD 11)
+# set(CMAKE_CXX_STANDARD_REQUIRED ON)
# Do not use, for example, `-std=gnu++11`.
-set(CMAKE_CXX_EXTENSIONS OFF)
+# set(CMAKE_CXX_EXTENSIONS OFF)
+add_compile_options(/std:c++latest)
+add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
+add_definitions(-D_HAS_AUTO_PTR_ETC=1)
+add_definitions(-D_HAS_TR1_NAMESPACE=1)
+add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
+add_definitions(-D_SILENCE_CXX17_ITERATOR_BASE_CLASS_DEPRECATION_WARNING)
```
Unfortunately, it still repro'ed with this patch applied too, so I need to figure out what we missed.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On July 23, 2018, 12:35 a.m., Benjamin Bannier wrote:
> > > It is not yet known if the Autotools system will also need this patch. Do we want it added there anyway?
> >
> > I'd vote for enabling this on all platforms to simplify the build setup.
Ah, but that doesn't simplify it, it complicates it :P (I've never added a patch to Autotools).
> On July 23, 2018, 12:35 a.m., Benjamin Bannier wrote:
> > 3rdparty/googletest-release-1.8.0.patch
> > Lines 1 (patched)
> > <https://reviews.apache.org/r/67916/diff/1/?file=2059402#file2059402line1>
> >
> > This patch has landed upstream so let's just use the patch from upstream `f66ab00704cd47e4e63ef6d425ca14b9192aaebb` instead of referencing a PR. If we do that we should also update our commit message.
> >
> > `f66ab00704c` applied cleanly with some fuzziness for me, so if possible just use the actual upstream patch.
I believe that I did cherry-pick the merged upstream patch. This diff was generated from the cherry-picked commit, which is why the hash is not `f66ab00704cd47e4e63ef6d425ca14b9192aaebb`.
> On July 23, 2018, 12:35 a.m., Benjamin Bannier wrote:
> > 3rdparty/googletest-release-1.8.0.patch
> > Lines 1-10 (patched)
> > <https://reviews.apache.org/r/67916/diff/1/?file=2059402#file2059402line1>
> >
> > Thanks for including the git metadata with the patch! This is much easier to maintain than bare diffs we have in many other patches.
The commit hash isn't useful since it was a cherry-pick, but the message I thought was nice to have :) I think I just generated with `git format-patch`... how else do people generate patches?
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/#review206332
-----------------------------------------------------------
On July 13, 2018, 2:04 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> -----------------------------------------------------------
>
> (Updated July 13, 2018, 2:04 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
>
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/googletest-release-1.8.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67916/diff/1/
>
>
> Testing
> -------
>
> Clean build on Windows using CMake (which is the only place this patch currently applies). It is not yet known if the Autotools system will also need this patch. Do we want it added there anyway?
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/#review206332
-----------------------------------------------------------
Fix it, then Ship it!
> It is not yet known if the Autotools system will also need this patch. Do we want it added there anyway?
I'd vote for enabling this on all platforms to simplify the build setup.
3rdparty/googletest-release-1.8.0.patch
Lines 1 (patched)
<https://reviews.apache.org/r/67916/#comment289263>
This patch has landed upstream so let's just use the patch from upstream `f66ab00704cd47e4e63ef6d425ca14b9192aaebb` instead of referencing a PR. If we do that we should also update our commit message.
`f66ab00704c` applied cleanly with some fuzziness for me, so if possible just use the actual upstream patch.
3rdparty/googletest-release-1.8.0.patch
Lines 1-10 (patched)
<https://reviews.apache.org/r/67916/#comment289264>
Thanks for including the git metadata with the patch! This is much easier to maintain than bare diffs we have in many other patches.
- Benjamin Bannier
On July 13, 2018, 11:04 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> -----------------------------------------------------------
>
> (Updated July 13, 2018, 11:04 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
>
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/googletest-release-1.8.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67916/diff/1/
>
>
> Testing
> -------
>
> Clean build on Windows using CMake (which is the only place this patch currently applies). It is not yet known if the Autotools system will also need this patch. Do we want it added there anyway?
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67916: Patched Google Test with upstream bugfix.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67916/#review206082
-----------------------------------------------------------
PASS: Mesos patch 67916 was successfully built and tested.
Reviews applied: `['67916']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1923/mesos-review-67916
- Mesos Reviewbot Windows
On July 13, 2018, 2:04 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> -----------------------------------------------------------
>
> (Updated July 13, 2018, 2:04 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
>
>
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
>
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/googletest-release-1.8.0.patch PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67916/diff/1/
>
>
> Testing
> -------
>
> Clean build on Windows using CMake (which is the only place this patch currently applies). It is not yet known if the Autotools system will also need this patch. Do we want it added there anyway?
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>