Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

[Srinivas Brahmaroutu]

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

(Updated June 3, 2016, 3:45 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.


Repository: mesos


Description
---

Add runtime for Appc Spec ex: command, workingdir and environment.


Diffs (updated)
-

  include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
  include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
  src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 
  src/slave/containerizer/mesos/containerizer.cpp 
c7b9744463cf8e1921dcb5e2b7dec7d4e2c0e45f 
  src/slave/containerizer/mesos/isolators/appc/runtime.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
aaa0efe63e587b9e604082b52a3cb8c11545fbb9 
  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
48a05059969e068a0ee0d38b61be9e7104e3188d 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
7540be6d8a412eb3d380d315c59223236d3eff67 
  src/slave/containerizer/mesos/provisioner/store.hpp 
1d477ef13ddd24fd8badae0decaa4a2271ecc746 
  src/tests/containerizer/provisioner_appc_tests.cpp 
84fe52b6937c3b7d7628b17a2f045eec2f386b4d 

Diff: https://reviews.apache.org/r/46498/diff/


Testing
---


Thanks,

Srinivas Brahmaroutu

Re: Review Request 46107: Add Appc runtime spec for command, working directory and environment.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46107]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On June 2, 2016, 10:14 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46107/
> ---
> 
> (Updated June 2, 2016, 10:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Appc runtime spec for command, working directory and environment.
> 
> 
> Diffs
> -
> 
>   include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
>   include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
> 
> Diff: https://reviews.apache.org/r/46107/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 46107: Add Appc runtime spec for command, working directory and environment.

[Srinivas Brahmaroutu]

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

(Updated June 2, 2016, 10:14 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

Add Appc runtime spec for command, working directory and environment.


Diffs (updated)
-

  include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
  include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 

Diff: https://reviews.apache.org/r/46107/diff/


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 45996: Fixed memory leak of `gc` in `finalize()` in libprocess.

[Neil Conway]

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

(Updated June 2, 2016, 8:30 p.m.)


Review request for mesos and Joris Van Remoortere.


Changes
---

Rebase.


Bugs: MESOS-5144
https://issues.apache.org/jira/browse/MESOS-5144


Repository: mesos


Description
---

Fixed memory leak of `gc` in `finalize()` in libprocess.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gc.hpp 
799468ebe49f2a49d325f40ffd8acea727abf74c 
  3rdparty/libprocess/src/process.cpp be120fb85583d438401c24a57dac803be3b5e5d2 

Diff: https://reviews.apache.org/r/45996/diff/


Testing
---


Thanks,

Neil Conway

Re: Review Request 45991: Fixed memory leak in Route::Route() in libprocess.

[Neil Conway]

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

(Updated June 2, 2016, 8:30 p.m.)


Review request for mesos and Joris Van Remoortere.


Changes
---

Rebase.


Bugs: MESOS-5144
https://issues.apache.org/jira/browse/MESOS-5144


Repository: mesos


Description
---

Fixed memory leak in Route::Route() in libprocess.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp be120fb85583d438401c24a57dac803be3b5e5d2 

Diff: https://reviews.apache.org/r/45991/diff/


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 45995: Fixed memory leak of `Route` in `finalize()` in libprocess.

[Neil Conway]

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

(Updated June 2, 2016, 8:31 p.m.)


Review request for mesos and Joris Van Remoortere.


Changes
---

Rebase.


Bugs: MESOS-5144
https://issues.apache.org/jira/browse/MESOS-5144


Repository: mesos


Description
---

Fixed memory leak of `Route` in `finalize()` in libprocess.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp be120fb85583d438401c24a57dac803be3b5e5d2 

Diff: https://reviews.apache.org/r/45995/diff/


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 46146: Fixed libprocess tests to use smart pointers.

[Neil Conway]

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

(Updated June 2, 2016, 8:30 p.m.)


Review request for mesos, Benjamin Bannier and Joris Van Remoortere.


Changes
---

Rebase.


Repository: mesos


Description
---

This ensures that dynamically allocated memory is released when
a test aborts prematurely due to an EXPECT_XXX failure.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/decoder_tests.cpp 
bd990c5eb77e47d7f617199bcc89e9432af7cc51 
  3rdparty/libprocess/src/tests/process_tests.cpp 
3fce6fc41faaa7b6e8a2a957e85de3de973a51ba 
  3rdparty/libprocess/src/tests/sequence_tests.cpp 
22863121711227cb0cab64b324327bd4eef1f149 

Diff: https://reviews.apache.org/r/46146/diff/


Testing
---

make check on OSX and Linux.


Thanks,

Neil Conway

Re: Review Request 46094: Fixed memory leaks in Encoder/Decoder tests in libprocess.

[Neil Conway]

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

(Updated June 2, 2016, 8:30 p.m.)


Review request for mesos, Joris Van Remoortere and Joseph Wu.


Changes
---

Rebase.


Repository: mesos


Description
---

Fixed memory leaks in Encoder/Decoder tests in libprocess.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/decoder_tests.cpp 
bd990c5eb77e47d7f617199bcc89e9432af7cc51 
  3rdparty/libprocess/src/tests/encoder_tests.cpp 
9153ddaa6d15845ab0067947a98478b9cea266a8 

Diff: https://reviews.apache.org/r/46094/diff/


Testing
---

make check

Verified that the number of leaked allocations decreases (from ~129 to ~92) 
with this change. Obviously there are more leaks to investigate but at first 
glance they seem a bit more subtle.


Thanks,

Neil Conway

Re: Review Request 45999: Ensure GC is terminated last during libprocess shutdown.

[Neil Conway]

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

(Updated June 2, 2016, 8:30 p.m.)


Review request for mesos and Joris Van Remoortere.


Changes
---

Rebase.


Bugs: MESOS-5144
https://issues.apache.org/jira/browse/MESOS-5144


Repository: mesos


Description
---

Without this change, `finalize()` terminates processes in the order
that they happen to be found when iterating over the `processes`
map. That means that if the GarbageCollector process is terminated
while any GC-managed processes are still running, those processes
will not be GC'd (i.e., they will be leaked).

Fix this by skipping the garbage collector process when iterating
over `processes` in `finalize()`, and then only terminating it after
all other processes have been terminated.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp be120fb85583d438401c24a57dac803be3b5e5d2 

Diff: https://reviews.apache.org/r/45999/diff/


Testing
---

make check

Confirmed reduction of leaked memory via ASAN on Linux/amd64.


Thanks,

Neil Conway

Re: Review Request 48183: Documented some suggestions for using persistent volume IDs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [48183]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On June 2, 2016, 7:09 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48183/
> ---
> 
> (Updated June 2, 2016, 7:09 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented some suggestions for using persistent volume IDs.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md c70291ad2d58bacaf508dfd22d19c0f1a473d466 
> 
> Diff: https://reviews.apache.org/r/48183/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 48183: Documented some suggestions for using persistent volume IDs.

[Neil Conway]

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

Review request for mesos, Greg Mann and Jie Yu.


Repository: mesos


Description
---

Documented some suggestions for using persistent volume IDs.


Diffs
-

  docs/persistent-volume.md c70291ad2d58bacaf508dfd22d19c0f1a473d466 

Diff: https://reviews.apache.org/r/48183/diff/


Testing
---


Thanks,

Neil Conway

Re: Review Request 34882: let mesos to compile and run on arm64 servers

[Neil Conway]

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



This shouldn't be necessary to enable compilation on ARM any longer, right? If 
so, please mark this review request as discarded.

- Neil Conway


On June 1, 2015, 7:11 a.m., Dong Robin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34882/
> ---
> 
> (Updated June 1, 2015, 7:11 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-2786
> https://issues.apache.org/jira/browse/MESOS-2786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To build and run mesos on arm64(aarch64) servers, we add
> the missing include header file, or it will report:
> "pivot_root is not available"
> 
> 
> Diffs
> -
> 
>   3rdparty/leveldb.patch ad8c19b9caa856ff85978ba832d48df11b3a83f0 
>   3rdparty/zookeeper-3.4.5.patch 3ca180d0c81f5de521ada7fb6c1c248a871ab2da 
>   src/linux/fs.cpp 1c9cf3f2ffead37148e4f6a81cefdbb97f679b09 
> 
> Diff: https://reviews.apache.org/r/34882/diff/
> 
> 
> Testing
> ---
> 
> Build and run mesos-master/mesos slave successfully on arm64 server.
> 
> 
> Thanks,
> 
> Dong Robin
> 
>

Re: Review Request 34881: let libprocess to compile on arm64 servers

[Neil Conway]

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



Seems like all of these changes should now be unnecessary, now that the 
third-party dependencies have been upgraded to more recent versions. Can you 
discard this review request if that is the case? Thanks.

- Neil Conway


On June 1, 2015, 7:10 a.m., Dong Robin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34881/
> ---
> 
> (Updated June 1, 2015, 7:10 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-2786
> https://issues.apache.org/jira/browse/MESOS-2786
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To compile libprocess on arm64(aarch64) servers, we need to
> add patches for 3rd software to recognize aarch64 architecture
> and replace x86 assemble language to standard gcc buildin function
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> 519e38c2c22904e75f36b936142a915a8f615b21 
>   3rdparty/libprocess/3rdparty/glog-0.3.3.patch 
> 76b8c0fe3b4615371e265bab713d62c896b7c3d6 
>   3rdparty/libprocess/3rdparty/libev-4.15.patch 
> bbd83e6928e6caba3bc5a9739823d60923cfaa2a 
>   3rdparty/libprocess/3rdparty/protobuf-2.5.0.patch PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34881/diff/
> 
> 
> Testing
> ---
> 
> Build and run mesos-mater/mesos-slave succesly on arm64 server.
> 
> 
> Thanks,
> 
> Dong Robin
> 
>

Re: Review Request 45574: Add `PerfEventSubsystem` for cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp (line 101)


Will we reach here?


- Qian Zhang


On April 16, 2016, 6:28 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45574/
> ---
> 
> (Updated April 16, 2016, 6:28 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5047
> https://issues.apache.org/jira/browse/MESOS-5047
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `PerfEventSubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp 674d5f886ea41b939a8e48832ee6595a78b2f6ce 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45574/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 48164: Suppress all warnings inside googlemock.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On June 2, 2016, 12:13 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48164/
> ---
> 
> (Updated June 2, 2016, 12:13 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Current clang-3.9.0 `be8f8b5` will always perform a (mostly) syntactic
> check that no direct `nullptr` dereferences are performed (emitted as
> `-Wnull-dereference`). Dereferencing a `nullptr` is undefined
> behavior.
> 
> The bundled googlemock sources contain such an instance when this
> template is instantiated (it is):
> 
> template 
> inline T Invalid() {
>   return const_cast::type&>(
>   *static_cast::type*>(NULL));
> }
> 
> Note that this was fixed upstream with `b5c81098` which was merged
> into googletest `master` on 2014-01-29, but not yet part of a released
> version (of which the last one was on 2013-09-19).
> 
> As mesos is compiled with `-Werror` and we apply the same kinds of
> warning filters to both mesos and googlemock, above code leads to
> mesos being rejected by current clang.
> 
> In this commit we change the way googlemock headers are added from
> `-I` to `-isystem`; the latter of these instructs the compiler to
> assume the added path contains system headers which suppresses all
> warnings originating from the headers found there.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 
> 
> Diff: https://reviews.apache.org/r/48164/diff/
> 
> 
> Testing
> ---
> 
> Tested on a number of Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46158: Completed implementation of the cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 97)


Why make it a static variable? I think `CgroupsIsolatorProcess::create()` 
will not be called more than once, right?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 105 - 106)


Since this is a multiple lines code, I think you need to add a newline 
after that, please check the following code as a reference:

https://github.com/apache/mesos/blob/0.28.1/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L285;L288
And you may need to do this change for all other places like this.

And what about just name this variable as `subsystem`?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 106)


I would suggest to use `string::PREFIX` since we are sure `cgroups/` is the 
prefix.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 146)


s/internal/internally/



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 148)


s/different with/be different from/



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 149)


s/is belongs to/belongs to/



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 155)


s/subsystem/Subsystem/


- Qian Zhang


On April 16, 2016, 6:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46158/
> ---
> 
> (Updated April 16, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Completed implementation of the cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46158/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 48141: Added --batch-mode to mvn invocation.

[Kapil Arya]

> On June 1, 2016, 5:52 p.m., Till Toenshoff wrote:
> > src/Makefile.am, line 1474
> > 
> >
> > Can we be sure that all Mesos supported versions of Maven actually 
> > support this flag?

It's been there since at least 2.2 release 
(https://git-wip-us.apache.org/repos/asf?p=maven.git;a=blob;f=maven-core/src/main/java/org/apache/maven/cli/CLIManager.java;h=0ea56a07ccf74d9cf2272db9c5da5e0cd3744061;hb=847203c6346f4b8d5ae27285f11eda8818af283f#l144).
 The 2.x already reached end-of-life so we need not worry :-).


- Kapil


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


On June 1, 2016, 4:21 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48141/
> ---
> 
> (Updated June 1, 2016, 4:21 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-5532
> https://issues.apache.org/jira/browse/MESOS-5532
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This prevents several thousands of additional lines related to maven
> artifact downloading.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 
> 
> Diff: https://reviews.apache.org/r/48141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 48164: Suppress all warnings inside googlemock.

[Benjamin Bannier]

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

(Updated June 2, 2016, 2:13 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Address Till's comment.


Repository: mesos


Description
---

Current clang-3.9.0 `be8f8b5` will always perform a (mostly) syntactic
check that no direct `nullptr` dereferences are performed (emitted as
`-Wnull-dereference`). Dereferencing a `nullptr` is undefined
behavior.

The bundled googlemock sources contain such an instance when this
template is instantiated (it is):

template 
inline T Invalid() {
  return const_cast::type&>(
  *static_cast::type*>(NULL));
}

Note that this was fixed upstream with `b5c81098` which was merged
into googletest `master` on 2014-01-29, but not yet part of a released
version (of which the last one was on 2013-09-19).

As mesos is compiled with `-Werror` and we apply the same kinds of
warning filters to both mesos and googlemock, above code leads to
mesos being rejected by current clang.

In this commit we change the way googlemock headers are added from
`-I` to `-isystem`; the latter of these instructs the compiler to
assume the added path contains system headers which suppresses all
warnings originating from the headers found there.


Diffs (updated)
-

  src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 

Diff: https://reviews.apache.org/r/48164/diff/


Testing
---

Tested on a number of Linux configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 48164: Suppress all warnings inside googlemock.

[Benjamin Bannier]

> On June 2, 2016, 11:36 a.m., Till Toenshoff wrote:
> > src/Makefile.am, line 2086
> > 
> >
> > Let's add a comment explaining this move, please.

I added the canonical explanation.


- Benjamin


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


On June 2, 2016, 11:31 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48164/
> ---
> 
> (Updated June 2, 2016, 11:31 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Current clang-3.9.0 `be8f8b5` will always perform a (mostly) syntactic
> check that no direct `nullptr` dereferences are performed (emitted as
> `-Wnull-dereference`). Dereferencing a `nullptr` is undefined
> behavior.
> 
> The bundled googlemock sources contain such an instance when this
> template is instantiated (it is):
> 
> template 
> inline T Invalid() {
>   return const_cast::type&>(
>   *static_cast::type*>(NULL));
> }
> 
> Note that this was fixed upstream with `b5c81098` which was merged
> into googletest `master` on 2014-01-29, but not yet part of a released
> version (of which the last one was on 2013-09-19).
> 
> As mesos is compiled with `-Werror` and we apply the same kinds of
> warning filters to both mesos and googlemock, above code leads to
> mesos being rejected by current clang.
> 
> In this commit we change the way googlemock headers are added from
> `-I` to `-isystem`; the latter of these instructs the compiler to
> assume the added path contains system headers which suppresses all
> warnings originating from the headers found there.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 
> 
> Diff: https://reviews.apache.org/r/48164/diff/
> 
> 
> Testing
> ---
> 
> Tested on a number of Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 48164: Suppress all warnings inside googlemock.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [48164]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On June 2, 2016, 9:31 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48164/
> ---
> 
> (Updated June 2, 2016, 9:31 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Current clang-3.9.0 `be8f8b5` will always perform a (mostly) syntactic
> check that no direct `nullptr` dereferences are performed (emitted as
> `-Wnull-dereference`). Dereferencing a `nullptr` is undefined
> behavior.
> 
> The bundled googlemock sources contain such an instance when this
> template is instantiated (it is):
> 
> template 
> inline T Invalid() {
>   return const_cast::type&>(
>   *static_cast::type*>(NULL));
> }
> 
> Note that this was fixed upstream with `b5c81098` which was merged
> into googletest `master` on 2014-01-29, but not yet part of a released
> version (of which the last one was on 2013-09-19).
> 
> As mesos is compiled with `-Werror` and we apply the same kinds of
> warning filters to both mesos and googlemock, above code leads to
> mesos being rejected by current clang.
> 
> In this commit we change the way googlemock headers are added from
> `-I` to `-isystem`; the latter of these instructs the compiler to
> assume the added path contains system headers which suppresses all
> warnings originating from the headers found there.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 
> 
> Diff: https://reviews.apache.org/r/48164/diff/
> 
> 
> Testing
> ---
> 
> Tested on a number of Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 48164: Suppress all warnings inside googlemock.

[Till Toenshoff]

> On June 2, 2016, 9:36 a.m., Till Toenshoff wrote:
> > Thanks for testing a supplying a neat workaround Benjamin!

s/a supplying/and supplying/


- Till


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


On June 2, 2016, 9:31 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48164/
> ---
> 
> (Updated June 2, 2016, 9:31 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Current clang-3.9.0 `be8f8b5` will always perform a (mostly) syntactic
> check that no direct `nullptr` dereferences are performed (emitted as
> `-Wnull-dereference`). Dereferencing a `nullptr` is undefined
> behavior.
> 
> The bundled googlemock sources contain such an instance when this
> template is instantiated (it is):
> 
> template 
> inline T Invalid() {
>   return const_cast::type&>(
>   *static_cast::type*>(NULL));
> }
> 
> Note that this was fixed upstream with `b5c81098` which was merged
> into googletest `master` on 2014-01-29, but not yet part of a released
> version (of which the last one was on 2013-09-19).
> 
> As mesos is compiled with `-Werror` and we apply the same kinds of
> warning filters to both mesos and googlemock, above code leads to
> mesos being rejected by current clang.
> 
> In this commit we change the way googlemock headers are added from
> `-I` to `-isystem`; the latter of these instructs the compiler to
> assume the added path contains system headers which suppresses all
> warnings originating from the headers found there.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 
> 
> Diff: https://reviews.apache.org/r/48164/diff/
> 
> 
> Testing
> ---
> 
> Tested on a number of Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 48164: Suppress all warnings inside googlemock.

[Till Toenshoff]

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


Fix it, then Ship it!




Thanks for testing a supplying a neat workaround Benjamin!


src/Makefile.am (line 2086)


Let's add a comment explaining this move, please.


- Till Toenshoff


On June 2, 2016, 9:31 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48164/
> ---
> 
> (Updated June 2, 2016, 9:31 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Current clang-3.9.0 `be8f8b5` will always perform a (mostly) syntactic
> check that no direct `nullptr` dereferences are performed (emitted as
> `-Wnull-dereference`). Dereferencing a `nullptr` is undefined
> behavior.
> 
> The bundled googlemock sources contain such an instance when this
> template is instantiated (it is):
> 
> template 
> inline T Invalid() {
>   return const_cast::type&>(
>   *static_cast::type*>(NULL));
> }
> 
> Note that this was fixed upstream with `b5c81098` which was merged
> into googletest `master` on 2014-01-29, but not yet part of a released
> version (of which the last one was on 2013-09-19).
> 
> As mesos is compiled with `-Werror` and we apply the same kinds of
> warning filters to both mesos and googlemock, above code leads to
> mesos being rejected by current clang.
> 
> In this commit we change the way googlemock headers are added from
> `-I` to `-isystem`; the latter of these instructs the compiler to
> assume the added path contains system headers which suppresses all
> warnings originating from the headers found there.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 
> 
> Diff: https://reviews.apache.org/r/48164/diff/
> 
> 
> Testing
> ---
> 
> Tested on a number of Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 48164: Suppress all warnings inside googlemock.

[Benjamin Bannier]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Current clang-3.9.0 `be8f8b5` will always perform a (mostly) syntactic
check that no direct `nullptr` dereferences are performed (emitted as
`-Wnull-dereference`). Dereferencing a `nullptr` is undefined
behavior.

The bundled googlemock sources contain such an instance when this
template is instantiated (it is):

template 
inline T Invalid() {
  return const_cast::type&>(
  *static_cast::type*>(NULL));
}

Note that this was fixed upstream with `b5c81098` which was merged
into googletest `master` on 2014-01-29, but not yet part of a released
version (of which the last one was on 2013-09-19).

As mesos is compiled with `-Werror` and we apply the same kinds of
warning filters to both mesos and googlemock, above code leads to
mesos being rejected by current clang.

In this commit we change the way googlemock headers are added from
`-I` to `-isystem`; the latter of these instructs the compiler to
assume the added path contains system headers which suppresses all
warnings originating from the headers found there.


Diffs
-

  src/Makefile.am c0be66ab28c452e217fe7c7ead8b1b3c283908cc 

Diff: https://reviews.apache.org/r/48164/diff/


Testing
---

Tested on a number of Linux configurations in internal CI.


Thanks,

Benjamin Bannier