mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 67916: Patched Google Test with upstream bugfix.
Date Tue, 31 Jul 2018 17:10:59 GMT


> 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
> 
>


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