Re: Review Request 56781: Fixed compilation on VS 2017.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56781/#review165913 --- Fix it, then Ship it! 3rdparty/libprocess/include/process/future.hpp (line 412) <https://reviews.apache.org/r/56781/#comment237757> Should this be `#endif // __WINDOWS__`? - Alex Clemmer On Feb. 17, 2017, 8:12 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56781/ > --- > > (Updated Feb. 17, 2017, 8:12 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Alex Clemmer, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > 3rdparty/libprocess/include/process/future.hpp > 819ee5ceb8f2900087c2a06d5b1df0a1aeb413f6 > > Diff: https://reviews.apache.org/r/56781/diff/ > > > Testing > --- > > Tested on VS 2015 and 2017. > > > Thanks, > > Michael Park > >
Re: Review Request 56593: Explicitly marked protobuf files as using v2 syntax.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56593/ --- (Updated Feb. 16, 2017, 2:58 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Added JIRA -- Joseph Wu. Bugs: MESOS-6138 https://issues.apache.org/jira/browse/MESOS-6138 Repository: mesos Description --- `protoc` will complain about `.proto` files that do not explicitly specify the version of PB syntax they're using. On Windows, this registers as a build warning. To ameliorate this, we explicitly specify the syntax in all `.proto` files. Diffs - include/mesos/agent/agent.proto 775c14b70c94c90d31d0123f45188fd29651f9f4 include/mesos/allocator/allocator.proto 9fc8baccb135c5aa58f64847307978139139a46e include/mesos/appc/spec.proto 2dbb3504596dda229c42353098879350c3a7c960 include/mesos/authentication/authentication.proto a24d53512a8ec27e97cf543cd64129c8a096ac67 include/mesos/authorizer/acls.proto fd25e5449ad659ee0269cfc3f47b9939ac022fb4 include/mesos/authorizer/authorizer.proto 8b860a3e8e0b1c660a8fefc97f10f5acc0501920 include/mesos/docker/spec.proto 734395cbde81bc4ff99f1310fc4b913beeb5b11f include/mesos/docker/v1.proto 209fb4afe5e4ea1cc776bfe9cb36865c9b05aaf0 include/mesos/docker/v2.proto 9b5bfb5d03ae8115c1b20a8c640640d6dbaa257d include/mesos/executor/executor.proto e746608176245bcabf265925111b8df9cab8ca55 include/mesos/fetcher/fetcher.proto 7f204e1a0b70ecdcd4247e1c0699585243a97b40 include/mesos/maintenance/maintenance.proto 4afae5c6958a5da91b3e649b496b9757d951ca0c include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017 include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a include/mesos/module/hook.proto f7b91f813f7919098763790211d3b90aed0b8340 include/mesos/module/module.proto 1c6c4b0762f5a4e71cf64fd9bc859e380222b591 include/mesos/oci/spec.proto f3083dd129fe0902760bc85aa93dfe6985358bcb include/mesos/quota/quota.proto 78e12d56c4f41613ec6b731d86093f48a23b4509 include/mesos/scheduler/scheduler.proto da2c67af41c9b9d4fdf6d79e84d0187cb9873ad3 include/mesos/slave/containerizer.proto c70d437ac3f8f813fcb71c04275cc8eeeb946065 include/mesos/slave/oversubscription.proto e7346089d5699194b761c81ab9610ad33d83ba55 include/mesos/state/state.proto 7a7d68e6abc85c0ead04f5771e878d10348f44b8 include/mesos/uri/uri.proto 55023b681665e712990d45ff4a1dbc4ccf64edbf include/mesos/v1/agent/agent.proto a98acb7b0c66f7e17dcecf61ed22bfde70a33651 include/mesos/v1/allocator/allocator.proto 093f18f554470b14905db157bcc1e33303423eaa include/mesos/v1/executor/executor.proto 754e62aee5f822526eb9661aa255444c3f84dd1d include/mesos/v1/maintenance/maintenance.proto 27f7c2b3460fe3a7e1a845cd962cb9979a9ccc0b include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 include/mesos/v1/quota/quota.proto 424e6b4bf0064d146cfeeb19de77f0cce67882b9 include/mesos/v1/scheduler/scheduler.proto 65ccfa76c7029b546533d123d3c9be136b8c9124 src/master/registry.proto eab9821b6d159d14bf62b347d6188608ae5c src/messages/flags.proto 08406b69acaa52eba58e6091b9c8b3bc03981044 src/messages/log.proto 12c2d83d88bb5b282bc44d49655a67e671b12ba2 src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 src/messages/state.proto 857a88873681c9cd7263d0063e8f17f08c5cd64b src/slave/containerizer/mesos/isolators/docker/volume/state.proto 4c5b03c1a802c177f64ea8bb1e31fa58603991bb src/slave/containerizer/mesos/isolators/network/cni/spec.proto 4b3850037ec01969cabf16b94745c1802bf4de62 src/slave/containerizer/mesos/provisioner/docker/message.proto c93c7a92ec152bd9747a70392adfe6a0e863e839 Diff: https://reviews.apache.org/r/56593/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56594: Explicitly marked protobuf files as using v2 syntax.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56594/ --- (Updated Feb. 16, 2017, 2:58 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Added JIRA -- Joseph Wu. Bugs: MESOS-6138 https://issues.apache.org/jira/browse/MESOS-6138 Repository: mesos Description --- `protoc` will complain about `.proto` files that do not explicitly specify the version of PB syntax they're using. On Windows, this registers as a build warning. To ameliorate this, we explicitly specify the syntax in all `.proto` files. Diffs - 3rdparty/stout/tests/protobuf_tests.proto 229ac256b087d55ecc95636254261ecd12829187 Diff: https://reviews.apache.org/r/56594/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56591: Stout: Removed MSVC compiler warnings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56591/ --- (Updated Feb. 15, 2017, 9:36 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's comments. Repository: mesos Description --- Stout: Removed MSVC compiler warnings. Diffs (updated) - 3rdparty/stout/include/stout/abort.hpp b7dba032f579ead83f4e04c942239e721632eeb4 3rdparty/stout/include/stout/duration.hpp 0a30a09678c20937caa6f094c3c63a326e357932 3rdparty/stout/include/stout/gzip.hpp 7040a3275370e7447e843c2471f35e2ba26166e4 3rdparty/stout/include/stout/os/windows/dup.hpp 1fc4d55393aa617d1b67178c945ac0af91c30c99 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION 3rdparty/stout/include/stout/os/windows/fd.hpp fddaaa54deaea5e6ef3947142870c7fef76e76aa 3rdparty/stout/include/stout/protobuf.hpp c0f03bc547cddba29309c429b34a0c1e6015c8ea 3rdparty/stout/include/stout/windows/os.hpp b5172fca96c4151f4b1ebb6d343022558f45fc34 Diff: https://reviews.apache.org/r/56591/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56591: Stout: Removed MSVC compiler warnings.
> On Feb. 15, 2017, 2:07 a.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/duration.hpp, lines 99-100 > > <https://reviews.apache.org/r/56591/diff/2/?file=1633052#file1633052line99> > > > > See: https://reviews.apache.org/r/53708/#comment229180 Note: There is a typo, I believe, in that suggested code. In this line: ``` t.tv_usec = (us() / MICROSECONDS) - (t.tv_sec * MILLISECONDS); ``` I think you want to suggest `ns() / MICROSECONDS` instead of `us() / MICROSECONDS`. Let's also add a comment explaining why we're computing these quantities manually. > On Feb. 15, 2017, 2:07 a.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/protobuf.hpp, lines 178-180 > > <https://reviews.apache.org/r/56591/diff/2/?file=1633057#file1633057line178> > > > > Hm... > > > > This is a `size_t` (unsigned int) to `int` implicit cast. This means, > > if the string is of size 2^31 or greater, we will be passing in a negative > > size to the constructor. > > > > In proto2, the maximum size of a protobuf is 64 MB, so we're unlikely > > to hit the upper limit. > > > > However, since it is possible to pass in an arbitrary string to this > > method, we should add: > > > > ``` > > CHECK_LE(value.size(), std::numeric_limits::max()); > > ``` > > ^ Possibly need a `static_cast` in the first argument above. Alright, good idea. We originally didn't do this because I figured it's safe for the reason you mentioned, but I agree that we should be extra cautious. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56591/#review165596 --- On Feb. 14, 2017, 9:05 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56591/ > --- > > (Updated Feb. 14, 2017, 9:05 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Stout: Removed MSVC compiler warnings. > > > Diffs > - > > 3rdparty/stout/include/stout/abort.hpp > b7dba032f579ead83f4e04c942239e721632eeb4 > 3rdparty/stout/include/stout/duration.hpp > 0a30a09678c20937caa6f094c3c63a326e357932 > 3rdparty/stout/include/stout/gzip.hpp > 7040a3275370e7447e843c2471f35e2ba26166e4 > 3rdparty/stout/include/stout/os/windows/dup.hpp > 1fc4d55393aa617d1b67178c945ac0af91c30c99 > 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/fd.hpp > fddaaa54deaea5e6ef3947142870c7fef76e76aa > 3rdparty/stout/include/stout/protobuf.hpp > c0f03bc547cddba29309c429b34a0c1e6015c8ea > 3rdparty/stout/include/stout/windows/os.hpp > b5172fca96c4151f4b1ebb6d343022558f45fc34 > > Diff: https://reviews.apache.org/r/56591/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Review Request 56702: Windows: Handle environment variable inheritance uniformly in Mesos.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56702/ --- Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. Repository: mesos Description --- This review is a planned regression on MESOS-6816. In other parts of the codebase, we copy all environment variables from the system before launching a process, overwriting user specified environment variables. This is not correct behavior, but a half-measure that is necessary, because Windows does not inherit environment variables by default. In this commit, we make this behavior uniform across all places where we are creating a process, because it is better for behavior to be consistent before we get around to fixing it. We do have concrete plans to come back and resolve this issue. Diffs - src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d src/slave/containerizer/mesos/launch.cpp 4dd81b47ca4654f5e783a4f2227834e938bc8bb3 Diff: https://reviews.apache.org/r/56702/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56505: Added Windows support for Docker executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56505/ --- (Updated Feb. 15, 2017, 10:58 a.m.) Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. Bugs: MESOS-3098 https://issues.apache.org/jira/browse/MESOS-3098 Repository: mesos Description --- This commit will introduce the changes to the Mesos Docker interfaces required to use the Docker executor to run Windows Containers in the Mesos agent. In particular, since Windows Containers use named pipes to connect do the Docker host, rather than a socket, we introduce the plumbing to default to a named pipe connection when invoking the `docker` command. This work constitutes the beginning of the end of the work that will eventually result in Mesos support of Windows Containers. This review is partially based on r/52364, which was the work of Daniel Pravat. Lastly, this review has a planned regression in MESOS-6816. In other parts of the codebase, we copy all environment variables from the system before launching a process, overwriting user specified environment variables. This is not correct behavior, but a half-measure that is necessary, because Windows does not inherit environment variables by default. In this commit, we make this behavior uniform across all places where we are creating a process, because it is better for behavior to be consistent before we get around to fixing it. We do have concrete plans to come back and resolve this issue. Diffs (updated) - src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 src/docker/docker.cpp 68042fd6f3b19c12cddd0d2e74ae064ceceb6a8c src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 src/slave/constants.hpp 1159ac3b1f8797a847a6f5679045434064a519e9 src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 src/tests/flags.hpp cab24162e26be9fe79c0c65f24e35e3c3d735906 Diff: https://reviews.apache.org/r/56505/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56505: Added Windows support for Docker executor.
> On Feb. 14, 2017, 11:24 p.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/launch.cpp, lines 101-105 > > <https://reviews.apache.org/r/56505/diff/3/?file=1633530#file1633530line101> > > > > Why is this removed from the `#ifdef`? It still isn't used anywhere on > > Windows. It's just me preparing to turn on container checkpointing. I'll take it out. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56505/#review165583 --- On Feb. 14, 2017, 7:06 p.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56505/ > --- > > (Updated Feb. 14, 2017, 7:06 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. > > > Bugs: MESOS-3098 > https://issues.apache.org/jira/browse/MESOS-3098 > > > Repository: mesos > > > Description > --- > > This commit will introduce the changes to the Mesos Docker interfaces > required to use the Docker executor to run Windows Containers in the > Mesos agent. In particular, since Windows Containers use named pipes to > connect do the Docker host, rather than a socket, we introduce the > plumbing to default to a named pipe connection when invoking the > `docker` command. > > This work constitutes the beginning of the end of the work that will > eventually result in Mesos support of Windows Containers. > > This review is partially based on r/52364, which was the work of Daniel > Pravat. > > Lastly, this review has a planned regression in MESOS-6816. In other > parts of the codebase, we copy all environment variables from the system > before launching a process, overwriting user specified environment > variables. This is not correct behavior, but a half-measure that is > necessary, because Windows does not inherit environment variables by > default. In this commit, we make this behavior uniform across all places > where we are creating a process, because it is better for behavior to be > consistent before we get around to fixing it. We do have concrete plans > to come back and resolve this issue. > > > Diffs > - > > src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 > src/docker/docker.cpp 68042fd6f3b19c12cddd0d2e74ae064ceceb6a8c > src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 > src/slave/constants.hpp 1159ac3b1f8797a847a6f5679045434064a519e9 > src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d > src/slave/containerizer/mesos/launch.hpp > 49eb9128ae5a1e4ec02e2b9d9e3cb67d7a8f7663 > src/slave/containerizer/mesos/launch.cpp > 4dd81b47ca4654f5e783a4f2227834e938bc8bb3 > src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 > src/tests/flags.hpp cab24162e26be9fe79c0c65f24e35e3c3d735906 > > Diff: https://reviews.apache.org/r/56505/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 56505: Added Windows support for Docker executor.
> On Feb. 14, 2017, 11:24 p.m., Joseph Wu wrote: > > src/docker/docker.hpp, lines 41-47 > > <https://reviews.apache.org/r/56505/diff/3/?file=1633524#file1633524line41> > > > > This isn't a default, as you can't change the value without breaking it. Per offline conversation, this actually is the default host prefix, as Windows Containers supports either `npipe://` or `tcp://`. We've agreed to add a comment to clarify, but keep the name as is. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56505/#review165583 --- On Feb. 14, 2017, 7:06 p.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56505/ > --- > > (Updated Feb. 14, 2017, 7:06 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. > > > Bugs: MESOS-3098 > https://issues.apache.org/jira/browse/MESOS-3098 > > > Repository: mesos > > > Description > --- > > This commit will introduce the changes to the Mesos Docker interfaces > required to use the Docker executor to run Windows Containers in the > Mesos agent. In particular, since Windows Containers use named pipes to > connect do the Docker host, rather than a socket, we introduce the > plumbing to default to a named pipe connection when invoking the > `docker` command. > > This work constitutes the beginning of the end of the work that will > eventually result in Mesos support of Windows Containers. > > This review is partially based on r/52364, which was the work of Daniel > Pravat. > > Lastly, this review has a planned regression in MESOS-6816. In other > parts of the codebase, we copy all environment variables from the system > before launching a process, overwriting user specified environment > variables. This is not correct behavior, but a half-measure that is > necessary, because Windows does not inherit environment variables by > default. In this commit, we make this behavior uniform across all places > where we are creating a process, because it is better for behavior to be > consistent before we get around to fixing it. We do have concrete plans > to come back and resolve this issue. > > > Diffs > - > > src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 > src/docker/docker.cpp 68042fd6f3b19c12cddd0d2e74ae064ceceb6a8c > src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 > src/slave/constants.hpp 1159ac3b1f8797a847a6f5679045434064a519e9 > src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d > src/slave/containerizer/mesos/launch.hpp > 49eb9128ae5a1e4ec02e2b9d9e3cb67d7a8f7663 > src/slave/containerizer/mesos/launch.cpp > 4dd81b47ca4654f5e783a4f2227834e938bc8bb3 > src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 > src/tests/flags.hpp cab24162e26be9fe79c0c65f24e35e3c3d735906 > > Diff: https://reviews.apache.org/r/56505/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Review Request 56689: Windows: Prevented crash if sandbox `stdout` file already exists.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56689/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- Subprocess currently provides a cross-platform API that allows the child process to redirect `stdin`, `stdout`, and `stderr`, with support for file paths, file descriptors, pipes, or Windows file `HANDLE`s. Currently, in the case of file paths, the Windows code will use `CreateFile` to open the file, with the semantics that it will error out if the file already exists (because of the `CREATE_NEW` flag), and explicitly specifying that it is ok to have multiple processes writing to the file (because of the `FILE_SHARE_WRITE` flag). The former of these is undesirable; libprocess should be able to proceed regardless of whether the file already exists. But this also causes bugs to the downstream, namely Mesos, in which a Fetcher might create the sandbox `stdout` folder, and then subsequently crash when it tries to open the `stdout` folder in the executor. In this commit, we change the `CREATE_NEW` flag to `OPEN_ALWAYS`, which has the semantics that we will create the file if it does not exist, and open it if it does. Additionally, since the documentation does not specify whether `FILE_SHARE_WRITE` also allows other processes to have read access, we additionally specify the flag `FILE_SHARE_READ` just to be sure we make no assumptions about who is able to read and write to these files. See comments for additional context on this point. Diffs - 3rdparty/libprocess/src/subprocess_windows.cpp 1b5ae5e3ee3c08d05afa1bc533542f42fe26eed0 Diff: https://reviews.apache.org/r/56689/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56504: Stout: Made `os::mktemp` fully cross-platform.
> On Feb. 14, 2017, 11:05 p.m., Joseph Wu wrote: > > When we switched `mkdtemp` (directory version), that exposed some odd > > behavior on OSX due to how lengthy OSX's default temporary directory is. > > The IO Switchboard tests there ran into a 108 character limit for Unix > > sockets. It appears that this patch does not have as many side effects (as > > we create fewer temporary files than we do temporary directories). I should have written that I tested this in OS X also. Thanks for being thorough! - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56504/#review165582 --- On Feb. 9, 2017, 5:57 p.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56504/ > --- > > (Updated Feb. 9, 2017, 5:57 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Stout: Made `os::mktemp` fully cross-platform. > > > Diffs > - > > 3rdparty/stout/include/stout/os/mktemp.hpp > 231234f7652937e042f49b13fe8fc3c7193d26e1 > > Diff: https://reviews.apache.org/r/56504/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 56505: Added Windows support for Docker executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56505/ --- (Updated Feb. 14, 2017, 7:06 p.m.) Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. Changes --- Add forgotten change to `launch.hpp` Bugs: MESOS-3098 https://issues.apache.org/jira/browse/MESOS-3098 Repository: mesos Description --- This commit will introduce the changes to the Mesos Docker interfaces required to use the Docker executor to run Windows Containers in the Mesos agent. In particular, since Windows Containers use named pipes to connect do the Docker host, rather than a socket, we introduce the plumbing to default to a named pipe connection when invoking the `docker` command. This work constitutes the beginning of the end of the work that will eventually result in Mesos support of Windows Containers. This review is partially based on r/52364, which was the work of Daniel Pravat. Lastly, this review has a planned regression in MESOS-6816. In other parts of the codebase, we copy all environment variables from the system before launching a process, overwriting user specified environment variables. This is not correct behavior, but a half-measure that is necessary, because Windows does not inherit environment variables by default. In this commit, we make this behavior uniform across all places where we are creating a process, because it is better for behavior to be consistent before we get around to fixing it. We do have concrete plans to come back and resolve this issue. Diffs (updated) - src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 src/docker/docker.cpp 68042fd6f3b19c12cddd0d2e74ae064ceceb6a8c src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 src/slave/constants.hpp 1159ac3b1f8797a847a6f5679045434064a519e9 src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d src/slave/containerizer/mesos/launch.hpp 49eb9128ae5a1e4ec02e2b9d9e3cb67d7a8f7663 src/slave/containerizer/mesos/launch.cpp 4dd81b47ca4654f5e783a4f2227834e938bc8bb3 src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 src/tests/flags.hpp cab24162e26be9fe79c0c65f24e35e3c3d735906 Diff: https://reviews.apache.org/r/56505/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55547: Windows: Standardize on win32 environment, transition away from CRT.
> On Jan. 24, 2017, 2:23 a.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/os/raw/environment.hpp, lines 108-120 > > <https://reviews.apache.org/r/55547/diff/2/?file=1605587#file1605587line108> > > > > Should we also move away from functions like `os::execvpe`? If so, we > > would be able to completely exclude `stout/raw/environment.hpp` from the > > Windows headers. > > > > `Envp` is currently only used in one location: > > > > https://github.com/apache/mesos/blob/f1d0cdf1db2a28fa44f7aded0c3760636c0a51de/src/slave/containerizer/mesos/launch.cpp#L659-L684 Yes, we should. This will probably happen when we start to transition away from the hand-rolled windows task launching in the Command Executor. It probably is out of scope for this review, though, much as I'd like to do it now. > On Jan. 24, 2017, 2:23 a.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/os/windows/environment.hpp, lines 22-40 > > <https://reviews.apache.org/r/55547/diff/2/?file=1605588#file1605588line22> > > > > We should refactor this code along with some very similar parsing logic > > here: > > > > https://github.com/apache/mesos/blob/f1d0cdf1db2a28fa44f7aded0c3760636c0a51de/3rdparty/libprocess/include/process/windows/subprocess.hpp#L90-L114 Per our discussion, we have agreed to clean this up later as part of a refactoring of subprocess. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55547/#review162754 --- On Feb. 14, 2017, 6:47 p.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55547/ > --- > > (Updated Feb. 14, 2017, 6:47 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Bugs: MESOS-5880 > https://issues.apache.org/jira/browse/MESOS-5880 > > > Repository: mesos > > > Description > --- > > Windows currently exposes two APIs for managing a process's environment > variables: a CRT API, and a win32 API. This commit will transition Stout > to use only the win32 API, and retire its use of the CRT APIs. > > There are many reasons for this, for example: > > * Stout currently uses both the CRT and win32 APIs, but they are > incompatible, and this causes real bugs. For example, because > `os::setenv` uses the win32 API, but `os::environment` uses the CRT > API, it is possible to set an environment variable and then later not > see it reflected in the environment. In Mesos this causes many bugs, > such as when we expect to set `LIBPROCESS_PORT` in a parent, then > spawn a health checker, this port doesn't get picked up. > * The CRT API is very old, and essentially unmaintained. It should not > be used unless it is necessary. > * It is generally easier to mirror the most common POSIX semantics of > environment APIs with the win32 API than it is with the CRT API. For > example, the Windows CRT implementation of `setenv` will delete an > environment variable if you pass an empty string as value, instead of > setting the value to be an empty string, like most modern POSIX > implementations. On the other hand, the win32 equivalent, > `SetEnvironmentVariable`, does implement this behavior. > > The effort to standardize the Windows code paths essentially involves > two things: > > (1) Removing `os::raw::environment` from Stout's Windows API. > > `os::raw::environment` is not used by the Windows codepaths, and (for > reasons above) we want to avoid is accidental use of `environ` on > Windows in the future, as well. > > While it is possible to simply implement `os::raw::environment` using > the win32 API `GetEnvironmentStrings`, these return fundamentally > different types, so the allocation logic would become more complex, and > the semantics of the function would have to change. Either the user > would have to allocate a `char**` for the environment, or Stout would > have to manage a `static char**`, or the function would have to allocate > memory for the user to `free`. All of these are at odds with the POSIX > semantics, and since this API is only used on POSIX paths, there is no > real advantage in this line of inquiry. > > (2) Splitting up the implementation of `os::environment`. > > The POSIX `environ` and Windows `GetEnvironmentStrings` are > fundamentally different types, and require mostly
Re: Review Request 55547: Windows: Standardize on win32 environment, transition away from CRT.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55547/ --- (Updated Feb. 14, 2017, 6:47 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's comments. Bugs: MESOS-5880 https://issues.apache.org/jira/browse/MESOS-5880 Repository: mesos Description --- Windows currently exposes two APIs for managing a process's environment variables: a CRT API, and a win32 API. This commit will transition Stout to use only the win32 API, and retire its use of the CRT APIs. There are many reasons for this, for example: * Stout currently uses both the CRT and win32 APIs, but they are incompatible, and this causes real bugs. For example, because `os::setenv` uses the win32 API, but `os::environment` uses the CRT API, it is possible to set an environment variable and then later not see it reflected in the environment. In Mesos this causes many bugs, such as when we expect to set `LIBPROCESS_PORT` in a parent, then spawn a health checker, this port doesn't get picked up. * The CRT API is very old, and essentially unmaintained. It should not be used unless it is necessary. * It is generally easier to mirror the most common POSIX semantics of environment APIs with the win32 API than it is with the CRT API. For example, the Windows CRT implementation of `setenv` will delete an environment variable if you pass an empty string as value, instead of setting the value to be an empty string, like most modern POSIX implementations. On the other hand, the win32 equivalent, `SetEnvironmentVariable`, does implement this behavior. The effort to standardize the Windows code paths essentially involves two things: (1) Removing `os::raw::environment` from Stout's Windows API. `os::raw::environment` is not used by the Windows codepaths, and (for reasons above) we want to avoid is accidental use of `environ` on Windows in the future, as well. While it is possible to simply implement `os::raw::environment` using the win32 API `GetEnvironmentStrings`, these return fundamentally different types, so the allocation logic would become more complex, and the semantics of the function would have to change. Either the user would have to allocate a `char**` for the environment, or Stout would have to manage a `static char**`, or the function would have to allocate memory for the user to `free`. All of these are at odds with the POSIX semantics, and since this API is only used on POSIX paths, there is no real advantage in this line of inquiry. (2) Splitting up the implementation of `os::environment`. The POSIX `environ` and Windows `GetEnvironmentStrings` are fundamentally different types, and require mostly different processing logic to transform them to a `hashmap`. There is no real advantage in convoluting this processing code to keep the code common between them. Diffs (updated) - 3rdparty/stout/include/Makefile.am 4bde2ef3f466ed91c6922b330f07f5d425398751 3rdparty/stout/include/stout/os/environment.hpp d8c34999829257bee80b0678f2ee096f4798c62b 3rdparty/stout/include/stout/os/posix/environment.hpp PRE-CREATION 3rdparty/stout/include/stout/os/raw/environment.hpp b3e82ac8071b41748aeb098b7d5fcc210a1d3c43 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION 3rdparty/stout/tests/os_tests.cpp fc359159afcdb60e4406821e123b6358320b6df8 Diff: https://reviews.apache.org/r/55547/diff/ Testing --- Thanks, Alex Clemmer
Review Request 56675: Silence MSVC compiler warnings in libmesos.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56675/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- Silence MSVC compiler warnings in libmesos. Diffs - src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee src/common/values.cpp cb26627bb79e0c952e5fb5d216264719199372d3 src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a src/master/allocator/sorter/drf/sorter.cpp ae49fcd659972f984238f8f90aa395e345bfaaa6 src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72 src/slave/containerizer/mesos/containerizer.cpp d2b4f75a55dbe4746bc2dfc180335fa831a554ef src/slave/containerizer/mesos/isolators/posix.hpp 627004663cbd7223a252b875c51454a20e645be6 src/slave/containerizer/mesos/launcher.cpp 5114c130efbfb252dde1e85c081f5174e66f57af src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382 src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5 src/tests/protobuf_io_tests.cpp 82bffb814a979eceb46531f981053495c7b2bdbc src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f src/tests/scheduler_tests.cpp 1ff423fd5abaffc0e6a0f7fb0edc3e1d5c85fb59 src/tests/slave_tests.cpp b6f76824c20c842d8f1d8afb1f9b81668b6741da src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8 Diff: https://reviews.apache.org/r/56675/diff/ Testing --- Thanks, Alex Clemmer
Review Request 56652: Windows: Change ZK patch to fix macro redefinition warnings on MSVC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56652/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- Currently, compiling against ZK on Windows will cause many macro redefinition errors. This occurs because ZK has a `winstdint.h` file, which contains definitions for macros that historically have not existed in the MSVC toolchain. However, since MSVC 1900, many of these now exist. Here, we introduce a patch file that will condition out these definitions for versions 1900 and later. Additionally, the previous patch file was not fully correct, resulting in the patch being "fuzzed" when it was applied. This patch generates the correct output, allowing the patches to apply cleanly. Diffs - 3rdparty/zookeeper-06d3f3f.patch be2ceaf529895c92dcf53984d6d88c78fb1d74ec Diff: https://reviews.apache.org/r/56652/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56591: Stout: Removed MSVC compiler warnings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56591/ --- (Updated Feb. 14, 2017, 9:05 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Removed unnecessary comments. Repository: mesos Description --- Stout: Removed MSVC compiler warnings. Diffs (updated) - 3rdparty/stout/include/stout/abort.hpp b7dba032f579ead83f4e04c942239e721632eeb4 3rdparty/stout/include/stout/duration.hpp 0a30a09678c20937caa6f094c3c63a326e357932 3rdparty/stout/include/stout/gzip.hpp 7040a3275370e7447e843c2471f35e2ba26166e4 3rdparty/stout/include/stout/os/windows/dup.hpp 1fc4d55393aa617d1b67178c945ac0af91c30c99 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION 3rdparty/stout/include/stout/os/windows/fd.hpp fddaaa54deaea5e6ef3947142870c7fef76e76aa 3rdparty/stout/include/stout/protobuf.hpp c0f03bc547cddba29309c429b34a0c1e6015c8ea 3rdparty/stout/include/stout/windows/os.hpp b5172fca96c4151f4b1ebb6d343022558f45fc34 Diff: https://reviews.apache.org/r/56591/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56592: Libprocess: Removed MSVC compiler warnings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56592/ --- (Updated Feb. 14, 2017, 8:58 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Removed more warnings. Repository: mesos Description --- Libprocess: Removed MSVC compiler warnings. Diffs (updated) - 3rdparty/libprocess/include/process/metrics/counter.hpp a13cc7e18c8b23eae83c326d63874d9d2aaedc0d 3rdparty/libprocess/include/process/network.hpp 0590e7a67275b9e37af2a49c050daab5eeaee7a5 3rdparty/libprocess/src/encoder.hpp b019bf90c0aabbce50d90f5ed6f3fd25d725e542 Diff: https://reviews.apache.org/r/56592/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55749: Added CMake to standard documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55749/ --- (Updated Feb. 14, 2017, 2:18 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Split CMake documentation out to its own file. Bugs: MESOS-3139 https://issues.apache.org/jira/browse/MESOS-3139 Repository: mesos Description --- Added CMake to standard documentation. Diffs (updated) - docs/configuration-cmake.md PRE-CREATION docs/configuration.md 656aaa34915eaee91d388febbc7574287b9f51b5 docs/home.md 091dae20b0a26aac03186edaefab93c54b3063b4 Diff: https://reviews.apache.org/r/55749/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55030: CMake: Added source groups for libprocess build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55030/ --- (Updated Feb. 13, 2017, 11:04 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, and Joseph Wu. Changes --- Rebase to account for changes in `master`. Bugs: MESOS-3542 https://issues.apache.org/jira/browse/MESOS-3542 Repository: mesos Description --- Currently in IDEs such as XCode and Visual Studio, all mesos source files are grouped into one huge folder, with no attempt made to reflect the hierarchy of folders that exist on disk. This commit groups libprocess files into relevant source groups so that they appear in a sensible directory structure in these IDEs. In addition to beginning to directly address MESOS-3542, this is also (indirectly) the first step towards addressing the problem of breaking libmesos up into smaller binaries (MESOS-3542). Diffs (updated) - 3rdparty/libprocess/src/CMakeLists.txt ab362aa476e528deef691239a70f0be9d864e6c0 3rdparty/libprocess/src/tests/CMakeLists.txt c65c7f5a97817f4a790136e97eb9f02c0bf810b9 Diff: https://reviews.apache.org/r/55030/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56505: Added Windows support for Docker executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56505/ --- (Updated Feb. 13, 2017, 11:02 p.m.) Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. Changes --- Add planned regression for MESOS-6816 Bugs: MESOS-3098 https://issues.apache.org/jira/browse/MESOS-3098 Repository: mesos Description (updated) --- This commit will introduce the changes to the Mesos Docker interfaces required to use the Docker executor to run Windows Containers in the Mesos agent. In particular, since Windows Containers use named pipes to connect do the Docker host, rather than a socket, we introduce the plumbing to default to a named pipe connection when invoking the `docker` command. This work constitutes the beginning of the end of the work that will eventually result in Mesos support of Windows Containers. This review is partially based on r/52364, which was the work of Daniel Pravat. Lastly, this review has a planned regression in MESOS-6816. In other parts of the codebase, we copy all environment variables from the system before launching a process, overwriting user specified environment variables. This is not correct behavior, but a half-measure that is necessary, because Windows does not inherit environment variables by default. In this commit, we make this behavior uniform across all places where we are creating a process, because it is better for behavior to be consistent before we get around to fixing it. We do have concrete plans to come back and resolve this issue. Diffs (updated) - src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 src/docker/docker.cpp 68042fd6f3b19c12cddd0d2e74ae064ceceb6a8c src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 src/slave/constants.hpp 1159ac3b1f8797a847a6f5679045434064a519e9 src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d src/slave/containerizer/mesos/launch.cpp 4dd81b47ca4654f5e783a4f2227834e938bc8bb3 src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 src/tests/flags.hpp cab24162e26be9fe79c0c65f24e35e3c3d735906 Diff: https://reviews.apache.org/r/56505/diff/ Testing --- Thanks, Alex Clemmer
Review Request 56594: Explicitly marked protobuf files as using v2 syntax.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56594/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- `protoc` will complain about `.proto` files that do not explicitly specify the version of PB syntax they're using. On Windows, this registers as a build warning. To ameliorate this, we explicitly specify the syntax in all `.proto` files. Diffs - 3rdparty/stout/tests/protobuf_tests.proto 229ac256b087d55ecc95636254261ecd12829187 Diff: https://reviews.apache.org/r/56594/diff/ Testing --- Thanks, Alex Clemmer
Review Request 56593: Explicitly marked protobuf files as using v2 syntax.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56593/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- `protoc` will complain about `.proto` files that do not explicitly specify the version of PB syntax they're using. On Windows, this registers as a build warning. To ameliorate this, we explicitly specify the syntax in all `.proto` files. Diffs - include/mesos/agent/agent.proto 775c14b70c94c90d31d0123f45188fd29651f9f4 include/mesos/allocator/allocator.proto 9fc8baccb135c5aa58f64847307978139139a46e include/mesos/appc/spec.proto 2dbb3504596dda229c42353098879350c3a7c960 include/mesos/authentication/authentication.proto a24d53512a8ec27e97cf543cd64129c8a096ac67 include/mesos/authorizer/acls.proto fd25e5449ad659ee0269cfc3f47b9939ac022fb4 include/mesos/authorizer/authorizer.proto 8b860a3e8e0b1c660a8fefc97f10f5acc0501920 include/mesos/docker/spec.proto 734395cbde81bc4ff99f1310fc4b913beeb5b11f include/mesos/docker/v1.proto 209fb4afe5e4ea1cc776bfe9cb36865c9b05aaf0 include/mesos/docker/v2.proto 9b5bfb5d03ae8115c1b20a8c640640d6dbaa257d include/mesos/executor/executor.proto e746608176245bcabf265925111b8df9cab8ca55 include/mesos/fetcher/fetcher.proto 7f204e1a0b70ecdcd4247e1c0699585243a97b40 include/mesos/maintenance/maintenance.proto 4afae5c6958a5da91b3e649b496b9757d951ca0c include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017 include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a include/mesos/module/hook.proto f7b91f813f7919098763790211d3b90aed0b8340 include/mesos/module/module.proto 1c6c4b0762f5a4e71cf64fd9bc859e380222b591 include/mesos/oci/spec.proto f3083dd129fe0902760bc85aa93dfe6985358bcb include/mesos/quota/quota.proto 78e12d56c4f41613ec6b731d86093f48a23b4509 include/mesos/scheduler/scheduler.proto da2c67af41c9b9d4fdf6d79e84d0187cb9873ad3 include/mesos/slave/containerizer.proto c70d437ac3f8f813fcb71c04275cc8eeeb946065 include/mesos/slave/oversubscription.proto e7346089d5699194b761c81ab9610ad33d83ba55 include/mesos/state/state.proto 7a7d68e6abc85c0ead04f5771e878d10348f44b8 include/mesos/uri/uri.proto 55023b681665e712990d45ff4a1dbc4ccf64edbf include/mesos/v1/agent/agent.proto a98acb7b0c66f7e17dcecf61ed22bfde70a33651 include/mesos/v1/allocator/allocator.proto 093f18f554470b14905db157bcc1e33303423eaa include/mesos/v1/executor/executor.proto 754e62aee5f822526eb9661aa255444c3f84dd1d include/mesos/v1/maintenance/maintenance.proto 27f7c2b3460fe3a7e1a845cd962cb9979a9ccc0b include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 include/mesos/v1/quota/quota.proto 424e6b4bf0064d146cfeeb19de77f0cce67882b9 include/mesos/v1/scheduler/scheduler.proto 65ccfa76c7029b546533d123d3c9be136b8c9124 src/master/registry.proto eab9821b6d159d14bf62b347d6188608ae5c src/messages/flags.proto 08406b69acaa52eba58e6091b9c8b3bc03981044 src/messages/log.proto 12c2d83d88bb5b282bc44d49655a67e671b12ba2 src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 src/messages/state.proto 857a88873681c9cd7263d0063e8f17f08c5cd64b src/slave/containerizer/mesos/isolators/docker/volume/state.proto 4c5b03c1a802c177f64ea8bb1e31fa58603991bb src/slave/containerizer/mesos/isolators/network/cni/spec.proto 4b3850037ec01969cabf16b94745c1802bf4de62 src/slave/containerizer/mesos/provisioner/docker/message.proto c93c7a92ec152bd9747a70392adfe6a0e863e839 Diff: https://reviews.apache.org/r/56593/diff/ Testing --- Thanks, Alex Clemmer
Review Request 56592: Libprocess: Removed MSVC compiler warnings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56592/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- Libprocess: Removed MSVC compiler warnings. Diffs - 3rdparty/libprocess/include/process/metrics/counter.hpp a13cc7e18c8b23eae83c326d63874d9d2aaedc0d Diff: https://reviews.apache.org/r/56592/diff/ Testing --- Thanks, Alex Clemmer
Review Request 56591: Stout: Removed MSVC compiler warnings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56591/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- Stout: Removed MSVC compiler warnings. Diffs - 3rdparty/stout/include/stout/abort.hpp b7dba032f579ead83f4e04c942239e721632eeb4 3rdparty/stout/include/stout/duration.hpp 0a30a09678c20937caa6f094c3c63a326e357932 3rdparty/stout/include/stout/gzip.hpp 7040a3275370e7447e843c2471f35e2ba26166e4 3rdparty/stout/include/stout/os/windows/dup.hpp 1fc4d55393aa617d1b67178c945ac0af91c30c99 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION 3rdparty/stout/include/stout/os/windows/fd.hpp fddaaa54deaea5e6ef3947142870c7fef76e76aa 3rdparty/stout/include/stout/protobuf.hpp c0f03bc547cddba29309c429b34a0c1e6015c8ea 3rdparty/stout/include/stout/windows/os.hpp b5172fca96c4151f4b1ebb6d343022558f45fc34 Diff: https://reviews.apache.org/r/56591/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55543: Fail the build if %PreferredToolArchitecture% is not set to `x64`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55543/ --- (Updated Feb. 12, 2017, 1:08 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's comments. Bugs: MESOS-6720 https://issues.apache.org/jira/browse/MESOS-6720 Repository: mesos Description --- Before building Mesos on a Windows machine, it is necessary to set `%PreferredToolArchitecture%` to the value `x64`. This is necessary to work around (at least) two bugs in the MSVC backend: in particular, the linker can sometimes take hours or days to link `mesos-x.x.x.lib`, and the build system occasionally finds it self spuriously unable to find file `mesos-x.x.x.lib` to link against. These issues are well-known and documented (e.g., in the official Mesos "getting started" document), but it is better to simply refuse to build Mesos at all on Windows unless that environment variable is set. This commit will introduce such a check. Diffs (updated) - cmake/CompilationConfigure.cmake 560935b81603dc58c167918d36e2ae0a4060673d src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee src/slave/cmake/AgentConfigure.cmake 8d930d329048440d57b621fe8393b11912cdb27b Diff: https://reviews.apache.org/r/55543/diff/ Testing --- Thanks, Alex Clemmer
Review Request 56504: Stout: Made `os::mktemp` fully cross-platform.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56504/ --- Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. Repository: mesos Description --- Stout: Made `os::mktemp` fully cross-platform. Diffs - 3rdparty/stout/include/stout/os/mktemp.hpp 231234f7652937e042f49b13fe8fc3c7193d26e1 Diff: https://reviews.apache.org/r/56504/diff/ Testing --- Thanks, Alex Clemmer
Review Request 56505: Added Windows support for Docker executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56505/ --- Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. Bugs: MESOS-3098 https://issues.apache.org/jira/browse/MESOS-3098 Repository: mesos Description --- This commit will introduce the changes to the Mesos Docker interfaces required to use the Docker executor to run Windows Containers in the Mesos agent. In particular, since Windows Containers use named pipes to connect do the Docker host, rather than a socket, we introduce the plumbing to default to a named pipe connection when invoking the `docker` command. This work constitutes the beginning of the end of the work that will eventually result in Mesos support of Windows Containers. This review is partially based on r/52364, which was the work of Daniel Pravat. Diffs - src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 src/docker/docker.cpp 68042fd6f3b19c12cddd0d2e74ae064ceceb6a8c src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 src/slave/constants.hpp 1159ac3b1f8797a847a6f5679045434064a519e9 src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 src/tests/flags.hpp cab24162e26be9fe79c0c65f24e35e3c3d735906 Diff: https://reviews.apache.org/r/56505/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 56364: Windows: Stout: Rewrite Job Object wrappers.
> On Feb. 8, 2017, 2:26 a.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/windows/os.hpp, lines 718-721 > > <https://reviews.apache.org/r/56364/diff/1/?file=1625895#file1625895line718> > > > > This is a good default. But we need a way to toggle this behavior, > > such that the agent's death does not kill child jobs. > > > > i.e. A Windows version of ChildHook::SETSID I asked Andy to follow up this patch with a different changeset that decouples the life of the executor from the life of the agent. Since the agent already kills all executors when it dies, I think it makes sense to have one set of patches just adding Mesos Containers support to Windows, and one fixing the semantics of a dying Agent. Thoughts? - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56364/#review164619 --- On Feb. 7, 2017, 2:31 a.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56364/ > --- > > (Updated Feb. 7, 2017, 2:31 a.m.) > > > Review request for mesos, Alex Clemmer and Joseph Wu. > > > Bugs: MESOS-6892 > https://issues.apache.org/jira/browse/MESOS-6892 > > > Repository: mesos > > > Description > --- > > `os::create_job` now returns a `Try` instead of a raw > `HANDLE`, forcing ownership of the job object handle onto the caller > of the function. `create_job` requires a `std::string name` for the > job object, which is mapped from a PID using `os::name_job`. > > The assignment of a process to the job object is now done via > `Try os::assign_job(SharedHandle, pid_t)`. > > The equivalent of killing a process tree with job object semantics > is simply to terminate the job object. This is done via > `os::kill_job(SharedHandle)`. > > > Diffs > - > > 3rdparty/stout/include/stout/windows/os.hpp > b5172fca96c4151f4b1ebb6d343022558f45fc34 > > Diff: https://reviews.apache.org/r/56364/diff/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 56347: Stout: Explicitly delete `SharedHandle` default constructor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56347/#review164634 --- Ship it! Ship It! - Alex Clemmer On Feb. 6, 2017, 10:56 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56347/ > --- > > (Updated Feb. 6, 2017, 10:56 p.m.) > > > Review request for mesos, Alex Clemmer and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Stout: Explicitly delete `SharedHandle` default constructor. > > > Diffs > - > > 3rdparty/stout/include/stout/windows.hpp > 3bc973b88ea39948780df2ecc7c3692f7e07e1a9 > > Diff: https://reviews.apache.org/r/56347/diff/ > > > Testing > --- > > make && make check > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 56354: Fixed incorrect `CHECK_EQ`s in `WindowsFD`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56354/#review164425 --- Ship it! Ship It! - Alex Clemmer On Feb. 6, 2017, 11:23 p.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56354/ > --- > > (Updated Feb. 6, 2017, 11:23 p.m.) > > > Review request for mesos and Alex Clemmer. > > > Repository: mesos > > > Description > --- > > We store both the `CRT` and `HANDLE` in `WindowsFD` whether we construct > from `CRT` or `HANDLE` by using `_get_osfhandle` and `_open_osfhandle`. > > Thus, accessing the `CRT` via `crt()` does not require that it be in the > `CRT` state, but rather `CRT` or `HANDLE`, for example. > > > Diffs > - > > 3rdparty/stout/include/stout/os/windows/fd.hpp > 24d3661aad72817d8b9e3cd88fe6178ab60832bd > > Diff: https://reviews.apache.org/r/56354/diff/ > > > Testing > --- > > Ran `.\support\windows-build.bat` on a Windows VM. > > > Thanks, > > Michael Park > >
Re: Review Request 54595: Introduced an `os::dup` abstraction in stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54595/#review164304 --- Ship it! Ship It! - Alex Clemmer On Feb. 6, 2017, 2:14 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54595/ > --- > > (Updated Feb. 6, 2017, 2:14 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Introduced an `os::dup` abstraction in stout. > > > Diffs > - > > 3rdparty/stout/include/Makefile.am 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 > 3rdparty/stout/include/stout/os.hpp > ed6fec3ac1c1f9dfb0585178401f4b552822a0a1 > 3rdparty/stout/include/stout/os/dup.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/dup.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/dup.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54595/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54591: Introduced `WindowsFD` class which is analogous to an `int` in POSIX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54591/#review164303 --- Ship it! I greatly appreciate the goal of maintaining the integrity of the POSIX code paths by minimizing the changes required to make them work. So, while I have very serious reservations about doing things like implementing a `<` operator semantics on something that ends up being a `HANDLE` under the hood, for now, I think this is good enough, and accomplishes the goals that are important to the project. - Alex Clemmer On Feb. 6, 2017, 1:04 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54591/ > --- > > (Updated Feb. 6, 2017, 1:04 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > In POSIX the socket, pipe and a file are represented by the `int` type. > In Windows: > - A socket is kept in a `SOCKET` type (64 bit wide) > - A pipe or a WinAPI file descriptor in a `HANDLE` (64 bit wide) > - A CRT file descriptor in an `int` > > The `WindowsFD` class is a type that brings all of these things > together and behaves analogously to an `int` in POSIX. > > > Diffs > - > > 3rdparty/stout/include/Makefile.am 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 > 3rdparty/stout/include/stout/os.hpp > ed6fec3ac1c1f9dfb0585178401f4b552822a0a1 > 3rdparty/stout/include/stout/os/int_fd.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/fd.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54591/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54591: Introduced `WindowsFD` class which is analogous to an `int` in POSIX.
> On Feb. 5, 2017, 8:48 a.m., Alex Clemmer wrote: > > 3rdparty/stout/include/stout/os/windows/fd.hpp, line 59 > > <https://reviews.apache.org/r/54591/diff/7/?file=1624392#file1624392line59> > > > > I'm not super up on the semantics of these newfangled C++11 constructor > > things, but is there any particular reason we need this to be trivially > > constructible, or anything like that? Because, if there's not any > > significant gain, I think it's worth wondering whether having a default > > value is slightly dangerous. > > Michael Park wrote: > We probably don't need triviality, but to support the "Use it just as if > you would an `int`", we do need a default constructor. > If we don't allow default construction, either we don't use `int` on > Linux, or we just live with the fact that Windows builds > break if/when someone tries to say `int_fd fd; ... // initalize later`. I > think I'd rather just let it work. What would you prefer? Your case is comelling, let's mark it dropped and do as you suggest. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54591/#review164273 --- On Feb. 6, 2017, 1:04 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54591/ > --- > > (Updated Feb. 6, 2017, 1:04 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > In POSIX the socket, pipe and a file are represented by the `int` type. > In Windows: > - A socket is kept in a `SOCKET` type (64 bit wide) > - A pipe or a WinAPI file descriptor in a `HANDLE` (64 bit wide) > - A CRT file descriptor in an `int` > > The `WindowsFD` class is a type that brings all of these things > together and behaves analogously to an `int` in POSIX. > > > Diffs > - > > 3rdparty/stout/include/Makefile.am 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 > 3rdparty/stout/include/stout/os.hpp > ed6fec3ac1c1f9dfb0585178401f4b552822a0a1 > 3rdparty/stout/include/stout/os/int_fd.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/fd.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54591/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54601: Replaced `int` with `int_fd` in stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54601/#review164280 --- Ship it! Ship It! - Alex Clemmer On Feb. 5, 2017, 1:39 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54601/ > --- > > (Updated Feb. 5, 2017, 1:39 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Replaced `int` with `int_fd` in stout. > > > Diffs > - > > 3rdparty/stout/include/stout/net.hpp > 4803aeb2fc1c89d42a02c631ef0450d5053ea8a2 > 3rdparty/stout/include/stout/os/mktemp.hpp > 2dfd35605eb4202a5475fe0e0d2f1fd27690a2de > 3rdparty/stout/include/stout/os/open.hpp > 2a357926860b1523c51f12c7edee2babe6afbfa3 > 3rdparty/stout/include/stout/os/posix/socket.hpp > 041f00083c595c335146048c8685c4f96226b8e8 > 3rdparty/stout/include/stout/os/read.hpp > d0b657db5b7dc0c1e11edc13fd6830a9f6273867 > 3rdparty/stout/include/stout/os/socket.hpp > e9d9306e63aff2be083b4254fbf6f31c37edc424 > 3rdparty/stout/include/stout/os/sunos.hpp > e0398d25c0a5d0b55934594dd59b2629957ab921 > 3rdparty/stout/include/stout/os/touch.hpp > 84d49bb8adc2612e380f037fd42c47166d55f593 > 3rdparty/stout/include/stout/os/windows/close.hpp > 9f1566bff19ee872418bce8a43a119c4f0a70a7c > 3rdparty/stout/include/stout/os/windows/fcntl.hpp > f331cd324bc609f5b8081fa86d66d9947daf04f6 > 3rdparty/stout/include/stout/os/windows/fsync.hpp > e1c4868b02b320906f446af1d9371374e90651bc > 3rdparty/stout/include/stout/os/windows/ftruncate.hpp > a7f53ad2e8735b515590af84c0efce3edcc1bebf > 3rdparty/stout/include/stout/os/windows/read.hpp > 09190f97f33db5dfa023f937f8f2a4a62ed1ec15 > 3rdparty/stout/include/stout/os/windows/sendfile.hpp > d6358cc02c1eea9298907da1f74eb7eeaeec7d21 > 3rdparty/stout/include/stout/os/windows/shell.hpp > 46a0b1d1a52648710dd82188072c0f9f24ac272d > 3rdparty/stout/include/stout/os/windows/socket.hpp > c787341181da53abc5a3b2eadc76c85fef181310 > 3rdparty/stout/include/stout/os/windows/write.hpp > e0f2f0d4d4168105357d6f98f2dc2800124d37e4 > 3rdparty/stout/include/stout/os/write.hpp > 9bb9fbd4593866b6193a1e253e65852da0ff5ed5 > 3rdparty/stout/include/stout/protobuf.hpp > 80cb20f40a7ddd4309d27973eef9fca9e4052b64 > 3rdparty/stout/include/stout/windows.hpp > 82387049895066ed9a1dcc48d11c236199ee8146 > 3rdparty/stout/include/stout/windows/os.hpp > c123772ab018d327b02dacfb314b835b9f373cfc > 3rdparty/stout/tests/os/filesystem_tests.cpp > 22460842c0db5dc5b6effbc2bdfce043ed47db6d > 3rdparty/stout/tests/os/sendfile_tests.cpp > 17e10d92a0a004cb7ac83f4ff9c71844418a35b0 > 3rdparty/stout/tests/os_tests.cpp 31a4c8f1114fb7922c5819c3b844885e80e09906 > 3rdparty/stout/tests/path_tests.cpp > 8275e38a503e64d242da6be0fe6bd0a0c21869a3 > > Diff: https://reviews.apache.org/r/54601/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54762: Introduced an `os::pipe` abstraction to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54762/#review164279 --- Fix it, then Ship it! 3rdparty/stout/include/Makefile.am (line 129) <https://reviews.apache.org/r/54762/#comment235953> Probably want to line these slashes up? 3rdparty/stout/include/stout/os/windows/pipe.hpp (line 43) <https://reviews.apache.org/r/54762/#comment235954> This seems like it should be using `WindowsError` instead? `ErrnoError` is usually only for CRT functions, while `WindowsError` also captures win32 error codes. - Alex Clemmer On Feb. 5, 2017, 1:38 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54762/ > --- > > (Updated Feb. 5, 2017, 1:38 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Introduced an `os::pipe` abstraction to stout. > > > Diffs > - > > 3rdparty/stout/include/Makefile.am 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 > 3rdparty/stout/include/stout/os.hpp > ed6fec3ac1c1f9dfb0585178401f4b552822a0a1 > 3rdparty/stout/include/stout/os/pipe.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/pipe.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/pipe.hpp PRE-CREATION > 3rdparty/stout/include/stout/posix/os.hpp > 68df2ef5cda9416d2d3bfa94f05ac04bf35d663f > 3rdparty/stout/include/stout/windows/os.hpp > c123772ab018d327b02dacfb314b835b9f373cfc > > Diff: https://reviews.apache.org/r/54762/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54592: Introduced an `os::lseek` abstraction in stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54592/#review164278 --- Fix it, then Ship it! 3rdparty/stout/include/Makefile.am (line 83) <https://reviews.apache.org/r/54592/#comment235950> Should this be fixed? 3rdparty/stout/include/stout/os/lseek.hpp (line 22) <https://reviews.apache.org/r/54592/#comment235951> Should we include `try.hpp` and `error.hpp` here also? 3rdparty/stout/include/stout/os/lseek.hpp (line 26) <https://reviews.apache.org/r/54592/#comment235952> Since the Windows signature uses `long` instead of `off_t`, I wonder if it is appropriate to do a `static_assert` to verify they're appropriately convertable? - Alex Clemmer On Feb. 5, 2017, 1:38 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54592/ > --- > > (Updated Feb. 5, 2017, 1:38 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Introduced an `os::lseek` abstraction in stout. > > > Diffs > - > > 3rdparty/stout/include/Makefile.am 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 > 3rdparty/stout/include/stout/os.hpp > ed6fec3ac1c1f9dfb0585178401f4b552822a0a1 > 3rdparty/stout/include/stout/os/lseek.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54592/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54595: Introduced an `os::dup` abstraction in stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54595/#review164277 --- 3rdparty/stout/include/Makefile.am (line 68) <https://reviews.apache.org/r/54595/#comment235946> I think you need to fix this? 3rdparty/stout/include/stout/os/windows/dup.hpp (line 20) <https://reviews.apache.org/r/54595/#comment235948> Tiny nits: Should we `#include` `try.hpp` and `unreachable.hpp` also? Also, in this case, I thought standard practice was to include `stout/os/int_fd.hpp` instead of the platform-specific header? That's what we do everywhere else. 3rdparty/stout/include/stout/os/windows/dup.hpp (line 30) <https://reviews.apache.org/r/54595/#comment235947> Tiny nit: While this is not wrong _per se_, I think it is probably more correct to write `result == -1`. 3rdparty/stout/include/stout/os/windows/dup.hpp (line 37) <https://reviews.apache.org/r/54595/#comment235949> Tiny, tiny nit: we're prefixing other global-scope functions with `::`. Should this one be, too? - Alex Clemmer On Feb. 5, 2017, 1:38 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54595/ > --- > > (Updated Feb. 5, 2017, 1:38 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Introduced an `os::dup` abstraction in stout. > > > Diffs > - > > 3rdparty/stout/include/Makefile.am 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 > 3rdparty/stout/include/stout/os.hpp > ed6fec3ac1c1f9dfb0585178401f4b552822a0a1 > 3rdparty/stout/include/stout/os/dup.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/dup.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/dup.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54595/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54591: Introduced `WindowsFD` class which is analogous to an `int` in POSIX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54591/#review164276 --- 3rdparty/stout/include/stout/os.hpp (line 49) <https://reviews.apache.org/r/54591/#comment235945> Oh, also, don't we need to add all of these headers to the `Makefile.am`? - Alex Clemmer On Feb. 5, 2017, 1:36 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54591/ > --- > > (Updated Feb. 5, 2017, 1:36 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > In POSIX the socket, pipe and a file are represented by the `int` type. > In Windows: > - A socket is kept in a `SOCKET` type (64 bit wide) > - A pipe or a WinAPI file descriptor in a `HANDLE` (64 bit wide) > - A CRT file descriptor in an `int` > > The `WindowsFD` class is a type that brings all of these things > together and behaves analogously to an `int` in POSIX. > > > Diffs > - > > 3rdparty/stout/include/stout/os.hpp > ed6fec3ac1c1f9dfb0585178401f4b552822a0a1 > 3rdparty/stout/include/stout/os/int_fd.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/fd.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54591/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54591: Introduced `WindowsFD` class which is analogous to an `int` in POSIX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54591/#review164273 --- 3rdparty/stout/include/stout/os/windows/fd.hpp (line 20) <https://reviews.apache.org/r/54591/#comment235939> Should these be alphabetized? 3rdparty/stout/include/stout/os/windows/fd.hpp (line 59) <https://reviews.apache.org/r/54591/#comment235941> I'm not super up on the semantics of these newfangled C++11 constructor things, but is there any particular reason we need this to be trivially constructible, or anything like that? Because, if there's not any significant gain, I think it's worth wondering whether having a default value is slightly dangerous. 3rdparty/stout/include/stout/os/windows/fd.hpp (line 71) <https://reviews.apache.org/r/54591/#comment235940> Is it true that we're expecting `HANDLE`s passed to this class to only correspond to files? If not, I think it's worth noting that `INVALID_HANDLE_VALUE` is not the error value of all `HANDLE`s returned from the win32 APIs, _cf_. the "documentation" at [1]. Depending on the "type" of `HANDLE` returned, an error could be denoted by the handle being `== NULL`, `== -1`, and even `<= 32` in the case of `ShellExecute`. See [2]. If yes, I think it's worth at least documenting this as part of the class. (Honestly, I would prefer file `HANDLE`s be a different type entirely, but here we are.) [1] https://blogs.msdn.microsoft.com/oldnewthing/20040302-00/?p=40443 [2] http://stackoverflow.com/questions/3905538/testing-for-an-invalid-windows-handle-should-i-compare-with-null-0-or-even 3rdparty/stout/include/stout/os/windows/fd.hpp (line 86) <https://reviews.apache.org/r/54591/#comment235942> In `crt`, why not check the `type_` here and `abort` if the type is not compatible with what is requested, sort of like how we do in `Try`? It seems like it's better to replace a subtle error with an obvious one. I'm not sure if this also makes sense for the `operator`s below, too? 3rdparty/stout/include/stout/os/windows/fd.hpp (line 306) <https://reviews.apache.org/r/54591/#comment235944> I'm a bit confused about the conversion logic here... if `left` is a CRT type, can't we just `static_cast(left)` and compare that to the right? What am I missing? Also just so I'm clear: your comment above is saying that the check of `< 0` is just because `left` is signed, while `right` is unsigned, so they can't be equal in this case? Is it appropriate to do a `static_assert` to make sure `HANDLE` is unsigned in the future, too? 3rdparty/stout/include/stout/os/windows/fd.hpp (line 311) <https://reviews.apache.org/r/54591/#comment235943> Just for my own education, we are doing a `reinterpret_cast` here? Can we not just `static_cast`? - Alex Clemmer On Feb. 5, 2017, 1:36 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54591/ > --- > > (Updated Feb. 5, 2017, 1:36 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > In POSIX the socket, pipe and a file are represented by the `int` type. > In Windows: > - A socket is kept in a `SOCKET` type (64 bit wide) > - A pipe or a WinAPI file descriptor in a `HANDLE` (64 bit wide) > - A CRT file descriptor in an `int` > > The `WindowsFD` class is a type that brings all of these things > together and behaves analogously to an `int` in POSIX. > > > Diffs > - > > 3rdparty/stout/include/stout/os.hpp > ed6fec3ac1c1f9dfb0585178401f4b552822a0a1 > 3rdparty/stout/include/stout/os/int_fd.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/fd.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54591/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54601: Replaced `int` with `int_fd` in stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54601/#review164009 --- 3rdparty/stout/include/stout/os/mktemp.hpp (line 44) <https://reviews.apache.org/r/54601/#comment235526> This also seems to be not a correct comparison? See the comment in `io::read` in https://reviews.apache.org/r/54602/ 3rdparty/stout/include/stout/os/open.hpp (line 74) <https://reviews.apache.org/r/54601/#comment235527> This also seems to be not a correct comparison? See the comment in `io::read` in https://reviews.apache.org/r/54602/ - Alex Clemmer On Jan. 10, 2017, 2:49 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54601/ > --- > > (Updated Jan. 10, 2017, 2:49 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Replaced `int` with `int_fd` in stout. > > > Diffs > - > > 3rdparty/stout/include/stout/net.hpp > 4803aeb2fc1c89d42a02c631ef0450d5053ea8a2 > 3rdparty/stout/include/stout/os/mktemp.hpp > 2dfd35605eb4202a5475fe0e0d2f1fd27690a2de > 3rdparty/stout/include/stout/os/open.hpp > 2a357926860b1523c51f12c7edee2babe6afbfa3 > 3rdparty/stout/include/stout/os/posix/socket.hpp > 041f00083c595c335146048c8685c4f96226b8e8 > 3rdparty/stout/include/stout/os/read.hpp > d0b657db5b7dc0c1e11edc13fd6830a9f6273867 > 3rdparty/stout/include/stout/os/socket.hpp > e9d9306e63aff2be083b4254fbf6f31c37edc424 > 3rdparty/stout/include/stout/os/sunos.hpp > e0398d25c0a5d0b55934594dd59b2629957ab921 > 3rdparty/stout/include/stout/os/touch.hpp > 84d49bb8adc2612e380f037fd42c47166d55f593 > 3rdparty/stout/include/stout/os/windows/close.hpp > 9f1566bff19ee872418bce8a43a119c4f0a70a7c > 3rdparty/stout/include/stout/os/windows/fcntl.hpp > f331cd324bc609f5b8081fa86d66d9947daf04f6 > 3rdparty/stout/include/stout/os/windows/fsync.hpp > e1c4868b02b320906f446af1d9371374e90651bc > 3rdparty/stout/include/stout/os/windows/ftruncate.hpp > a7f53ad2e8735b515590af84c0efce3edcc1bebf > 3rdparty/stout/include/stout/os/windows/read.hpp > 09190f97f33db5dfa023f937f8f2a4a62ed1ec15 > 3rdparty/stout/include/stout/os/windows/sendfile.hpp > d6358cc02c1eea9298907da1f74eb7eeaeec7d21 > 3rdparty/stout/include/stout/os/windows/shell.hpp > 17e3d564564abebf1d558b7a7a277aef3c87e5ae > 3rdparty/stout/include/stout/os/windows/socket.hpp > c787341181da53abc5a3b2eadc76c85fef181310 > 3rdparty/stout/include/stout/os/windows/write.hpp > 23488708ae366b8571bb8b4805f67d2054223fff > 3rdparty/stout/include/stout/os/write.hpp > 9bb9fbd4593866b6193a1e253e65852da0ff5ed5 > 3rdparty/stout/include/stout/protobuf.hpp > 80cb20f40a7ddd4309d27973eef9fca9e4052b64 > 3rdparty/stout/include/stout/windows.hpp > e641c46d033372e1b6c9f9c066b1ad4957d55088 > 3rdparty/stout/include/stout/windows/os.hpp > 5cd92545a49648e39e8eb7cf131895e9cfc97902 > 3rdparty/stout/tests/os/filesystem_tests.cpp > 22460842c0db5dc5b6effbc2bdfce043ed47db6d > 3rdparty/stout/tests/os/sendfile_tests.cpp > 17e10d92a0a004cb7ac83f4ff9c71844418a35b0 > 3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 > 3rdparty/stout/tests/path_tests.cpp > 8275e38a503e64d242da6be0fe6bd0a0c21869a3 > > Diff: https://reviews.apache.org/r/54601/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 54602: Replaced `int` with `int_fd` in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54602/#review164008 --- 3rdparty/libprocess/src/io.cpp (line 222) <https://reviews.apache.org/r/54602/#comment235523> This comparison is definitely a bug. On Windows, when this `fd` is a pipe `HANDLE`, this comparison fails even for valid values of `fd`. This can, among other things, cause the Windows Executor to hang indefinitely in some scenarios. For example, when we call `docker inspect`, we will probably end up writing more data to the pipe than the pipe has buffer, so the Executor will block until we `io::read` it. But since this comparison fails, we will never complete the read. Hence, the Executor hangs indefinitely. This comparison can be made to work on Windows by making it properly check whether the `HANDLE` (using, _e.g._, `fd == os::WindowsFd(INVALID_HANDLE_VALUE)`) is invalid, but this obviously won't work on POSIX. In general, I suspect these comparisons are fraught, so I think we should consider removing them and having utility functions such as `fd.valid()` or something. 3rdparty/libprocess/src/io.cpp (line 283) <https://reviews.apache.org/r/54602/#comment235524> This also seems to be not a correct comparison? See the comment in `io::read`. 3rdparty/libprocess/src/process.cpp (line 1457) <https://reviews.apache.org/r/54602/#comment235525> This also seems to be not a correct comparison? See the comment in `io::read`. - Alex Clemmer On Jan. 10, 2017, 2:50 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54602/ > --- > > (Updated Jan. 10, 2017, 2:50 a.m.) > > > Review request for mesos, Daniel Pravat and Joris Van Remoortere. > > > Repository: mesos > > > Description > --- > > Replaced `int` with `int_fd` in libprocess. > > > Diffs > - > > 3rdparty/libprocess/include/process/io.hpp > f5489dc66adb136d9f53f98ac64c3fbe7831a1c2 > 3rdparty/libprocess/include/process/network.hpp > 8234765e23bb3d434da0b0f818fd42569d554ab8 > 3rdparty/libprocess/include/process/posix/subprocess.hpp > a1054e788ef6a322901c262380aceab8192235ac > 3rdparty/libprocess/include/process/socket.hpp > 87966155aa21328db51796b2ae0a883054c00457 > 3rdparty/libprocess/include/process/subprocess_base.hpp > 74a4bef0706334b4d3c1040a08a8921fbc300344 > 3rdparty/libprocess/include/process/windows/subprocess.hpp > 3bc7f1992d9c38dac2ec23d5bc57415f37d0318a > 3rdparty/libprocess/src/encoder.hpp > 1447d6f93c15b9f3d134507ecb0bda020a218a49 > 3rdparty/libprocess/src/http.cpp b5c38ce89b73788f7446e6c44fd99da6478b064d > 3rdparty/libprocess/src/io.cpp 8aa3576a11ed5e998492b4020cb37a45ec708093 > 3rdparty/libprocess/src/libev_poll.cpp > da2a78bd8aa4ed8c37bd2ca16d148b73112aa0e7 > 3rdparty/libprocess/src/libevent_poll.cpp > 0803ba33622c86df38b3efd4f1e3197edf93a0af > 3rdparty/libprocess/src/libevent_ssl_socket.hpp > 57eaf4f607d0628db466cc1a139772eeeaa51136 > 3rdparty/libprocess/src/libevent_ssl_socket.cpp > 0e292a8b0d470f4e199db08f09a0c863d73c > 3rdparty/libprocess/src/poll_socket.hpp > 89789e6bb91d79e4de1c4f4be3882df851845930 > 3rdparty/libprocess/src/poll_socket.cpp > 93ca37f105527fb225574107480114ee5ac74c76 > 3rdparty/libprocess/src/process.cpp > f475fe78f801924f70f51fdc4ab190c2dbecd656 > 3rdparty/libprocess/src/socket.cpp b819503095261c77f42d6f20d1a4b2b6170fb4e1 > 3rdparty/libprocess/src/subprocess.cpp > ad19b0896b4a2e9c60f573cc854c10c69e909e86 > 3rdparty/libprocess/src/subprocess_posix.cpp > 19271414f145d23f50ac07570c48782819f382b4 > 3rdparty/libprocess/src/subprocess_windows.cpp > 20cad52d4a4d7fc51487e150a849972eb19ed08e > 3rdparty/libprocess/src/tests/io_tests.cpp > 466e343e6d775fe09debce119eae4fc4849b7006 > 3rdparty/libprocess/src/tests/ssl_tests.cpp > a32d20e47f67d88bbe5928e0ddc129745c5f42e0 > 3rdparty/libprocess/src/tests/subprocess_tests.cpp > 59c17692012ddfb540ecdd48560c73c42a15f061 > > Diff: https://reviews.apache.org/r/54602/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Re: Review Request 55749: Added CMake to standard documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55749/ --- (Updated Jan. 29, 2017, 7:22 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Rearrange dependencies. Bugs: MESOS-3139 https://issues.apache.org/jira/browse/MESOS-3139 Repository: mesos Description --- Added CMake to standard documentation. Diffs - docs/configuration.md 2113d06f58dddc0a28ae1241a24096266fe39801 Diff: https://reviews.apache.org/r/55749/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55546: Added platform-independent constants `DEV_NULL` and `TRUE_COMMAND`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55546/ --- (Updated Jan. 29, 2017, 7:21 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Rearrange dependencies. Repository: mesos Description --- Added platform-independent constants `DEV_NULL` and `TRUE_COMMAND`. Diffs - 3rdparty/stout/include/stout/gtest.hpp a004a378cb467495234d77a0c56fbea6e7bec420 3rdparty/stout/include/stout/os/constants.hpp c71d52e1bc0433f9922f26a6eb486235bb9880d4 Diff: https://reviews.apache.org/r/55546/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55543: Fail the build if %PreferredToolArchitecture% is not set to `x64`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55543/ --- (Updated Jan. 29, 2017, 7:21 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Rearrange dependencies. Bugs: MESOS-6720 https://issues.apache.org/jira/browse/MESOS-6720 Repository: mesos Description --- Before building Mesos on a Windows machine, it is necessary to set `%PreferredToolArchitecture%` to the value `x64`. This is necessary to work around (at least) two bugs in the MSVC backend: in particular, the linker can sometimes take hours or days to link `mesos-x.x.x.lib`, and the build system occasionally finds it self spuriously unable to find file `mesos-x.x.x.lib` to link against. These issues are well-known and documented (e.g., in the official Mesos "getting started" document), but it is better to simply refuse to build Mesos at all on Windows unless that environment variable is set. This commit will introduce such a check. Diffs - cmake/CompilationConfigure.cmake 560935b81603dc58c167918d36e2ae0a4060673d Diff: https://reviews.apache.org/r/55543/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55162: Stout: Added style fixes and some useful error messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55162/ --- (Updated Jan. 29, 2017, 7:20 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Rearrange dependency. Repository: mesos Description --- Stout: Added style fixes and some useful error messages. Diffs - 3rdparty/stout/include/stout/os/windows/killtree.hpp 2be2f8c3d58ee64410f87ee4a4b2bb54fe014748 3rdparty/stout/include/stout/windows/os.hpp 5cd92545a49648e39e8eb7cf131895e9cfc97902 Diff: https://reviews.apache.org/r/55162/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55030: CMake: Added source groups for libprocess build.
> On Jan. 25, 2017, 12:29 a.m., Joseph Wu wrote: > > This review is partially un-done by https://reviews.apache.org/r/55604/ . > > Can you separate the source-grouping changes into > > https://reviews.apache.org/r/55604/ ? And leave the CMake lists > > rearrangement in this review? We still need this change because we need to add the `.hpp` files to the call to `add_executable` in order for them to show up as source groups. I've rebased to account for your recent changes. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55030/#review162883 --- On Jan. 29, 2017, 7:18 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55030/ > --- > > (Updated Jan. 29, 2017, 7:18 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, > and Joseph Wu. > > > Bugs: MESOS-3542 > https://issues.apache.org/jira/browse/MESOS-3542 > > > Repository: mesos > > > Description > --- > > Currently in IDEs such as XCode and Visual Studio, all mesos source > files are grouped into one huge folder, with no attempt made to reflect > the hierarchy of folders that exist on disk. > > This commit groups libprocess files into relevant source groups so that > they appear in a sensible directory structure in these IDEs. > > In addition to beginning to directly address MESOS-3542, this is also > (indirectly) the first step towards addressing the problem of breaking > libmesos up into smaller binaries (MESOS-3542). > > > Diffs > - > > 3rdparty/libprocess/src/CMakeLists.txt > ab362aa476e528deef691239a70f0be9d864e6c0 > 3rdparty/libprocess/src/tests/CMakeLists.txt > b298bbe042cabcb18169da1923562e9956374232 > > Diff: https://reviews.apache.org/r/55030/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55030: CMake: Added source groups for libprocess build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55030/ --- (Updated Jan. 29, 2017, 7:18 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, and Joseph Wu. Changes --- Rebase against current HEAD. Bugs: MESOS-3542 https://issues.apache.org/jira/browse/MESOS-3542 Repository: mesos Description --- Currently in IDEs such as XCode and Visual Studio, all mesos source files are grouped into one huge folder, with no attempt made to reflect the hierarchy of folders that exist on disk. This commit groups libprocess files into relevant source groups so that they appear in a sensible directory structure in these IDEs. In addition to beginning to directly address MESOS-3542, this is also (indirectly) the first step towards addressing the problem of breaking libmesos up into smaller binaries (MESOS-3542). Diffs (updated) - 3rdparty/libprocess/src/CMakeLists.txt ab362aa476e528deef691239a70f0be9d864e6c0 3rdparty/libprocess/src/tests/CMakeLists.txt b298bbe042cabcb18169da1923562e9956374232 Diff: https://reviews.apache.org/r/55030/diff/ Testing --- Thanks, Alex Clemmer
Review Request 56037: CMake: Bumped CMake version on Windows, and enforce with check.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56037/ --- Review request for mesos, Andrew Schwartzmeyer and Joseph Wu. Repository: mesos Description --- This commit will bump the required CMake version on Windows from ~3.5 to 3.6.3, and then add a configure-time check to ensure that this is true before we build. On Windows, bumping the required version is much less of a big deal than Unix, mainly because Windows does not have a package manager. The burden on developers is about the same, independent of the version, because the most convenient installation option is to use a `.msi`. Additionally, CMake is not widely used in the community, and where it is, the vast majority of users are building on Unix, so there are few constraints imposed by existing CI systems and users. The primary reason to bump the version, though, is that the CMake featureset on Windows tends (consciously or not) to straggle a bit behind the Unix featureset, and recently it has come to our attention that (due to our ignorance of this issue) we have come to depend on some subtle features that are available on Unix, but not on Windows, until CMake 3.6.3. Diffs - CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f docs/windows.md 5a5f934180f11fd6260032551f0df65fde541218 Diff: https://reviews.apache.org/r/56037/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55600: CMake: Transitioned Stout to automatic source grouping.
> On Jan. 25, 2017, 12:27 a.m., Joseph Wu wrote: > > 3rdparty/stout/cmake/StoutConfigure.cmake, line 47 > > <https://reviews.apache.org/r/55600/diff/2/?file=1606435#file1606435line47> > > > > Why not use `*.hpp` instead? > > Alex Clemmer wrote: > Generally, we also would like to include .h files generated by protobufs, > either those that exist, or those that might exist in the future. In Stout, > for example, there is `protobuf_tests.proto` in the `tests/` folder. The > Stout headers don't have any .proto files that I know of right now, but I > think it's better just to use this one pattern everywhere. > > Joseph Wu wrote: > Ok, but let's be explicit, say `*.h(pp)?`. Otherwise, we'll be matching > against `.hooligans` and `.hoopla` :) > > Similar below. > > Joseph Wu wrote: > Nevermind. I didn't realize this was a glob expression, rather than a > regex. Ah, yes. I would have done it that way if I could have made it work. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55600/#review162882 ------- On Jan. 17, 2017, 6:41 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55600/ > --- > > (Updated Jan. 17, 2017, 6:41 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Stout currently manually groups its source code by folder in order to > present the source nicely in IDEs like XCode and Visual Studio. > > With the recently-introduced CMake function, `GROUP_SOURCE`, this > process is automated away. This commit will remove these manual groups > and replace it with a call to this function. > > > Diffs > - > > 3rdparty/stout/cmake/StoutConfigure.cmake > bc27ac687bae4e1798eece562027ba33c6b32348 > 3rdparty/stout/tests/CMakeLists.txt > a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae > > Diff: https://reviews.apache.org/r/55600/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55607: CMake: Added configuration of test scripts in the bin/ directory.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55607/ --- (Updated Jan. 26, 2017, 9:20 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's questions. Bugs: MESOS-6757 https://issues.apache.org/jira/browse/MESOS-6757 Repository: mesos Description --- This resolves MESOS-6757. Diffs (updated) - cmake/MesosConfigure.cmake ce127e1e8c5c33ad8badef6fb3ac9f50ade9056f Diff: https://reviews.apache.org/r/55607/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55607: CMake: Added configuration of test scripts in the bin/ directory.
> On Jan. 25, 2017, 1:07 a.m., Joseph Wu wrote: > > cmake/MesosConfigure.cmake, line 217 > > <https://reviews.apache.org/r/55607/diff/2/?file=1606454#file1606454line217> > > > > How about moving the file instead? And cleaning up the bin/tmp folder? Unless I'm missing something, `COPY` is the preferred way to "move" a file, particularly if you want to set permissions. We can `REMOVE_RECURSE` to get rid of this `tmp/` folder though, and it should look basically identical to a move to a user. > On Jan. 25, 2017, 1:07 a.m., Joseph Wu wrote: > > cmake/MesosConfigure.cmake, line 212 > > <https://reviews.apache.org/r/55607/diff/2/?file=1606454#file1606454line212> > > > > This part ( > > https://cmake.org/cmake/help/v3.0/command/configure_file.html ): ``` > > If the file is modified the build system will re-run CMake to > > re-configure the file and generate the build system again. > > ``` > > > > is a little unfortunate, but that's better than the automake, which > > doesn't always regenerate these template files when the underlying ones get > > changed. It does suck, but I'm not sure what the alternatives are. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55607/#review162888 --- On Jan. 17, 2017, 8:34 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55607/ > --- > > (Updated Jan. 17, 2017, 8:34 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Bugs: MESOS-6757 > https://issues.apache.org/jira/browse/MESOS-6757 > > > Repository: mesos > > > Description > --- > > This resolves MESOS-6757. > > > Diffs > - > > cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc > > Diff: https://reviews.apache.org/r/55607/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55599: CMake: Added `GroupSource` function to automate IDE source grouping.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55599/ --- (Updated Jan. 26, 2017, 9:12 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's questions. Repository: mesos Description --- CMake allows users to declare groups of source files, which it uses to (among other things) present source in a directory-like tree of files in IDEs like XCode and Visual Studio. Currently this is a manual process: we group the source in each folder manually and declare it as a source group to CMake. This commit will introduce a CMake macro that automates this process away, providing control over many aspects, such as where the group tree should be rooted, what the root should be named, and so on. This macro indirectly partially addresses MESOS-3542, which will help us to separate out Mesos builds into many libraries, rather than one single, monolithic libmesos. Diffs (updated) - 3rdparty/stout/cmake/GroupSource.cmake PRE-CREATION Diff: https://reviews.apache.org/r/55599/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55600: CMake: Transitioned Stout to automatic source grouping.
> On Jan. 25, 2017, 12:27 a.m., Joseph Wu wrote: > > 3rdparty/stout/cmake/StoutConfigure.cmake, line 47 > > <https://reviews.apache.org/r/55600/diff/2/?file=1606435#file1606435line47> > > > > Why not use `*.hpp` instead? Generally, we also would like to include .h files generated by protobufs, either those that exist, or those that might exist in the future. In Stout, for example, there is `protobuf_tests.proto` in the `tests/` folder. The Stout headers don't have any .proto files that I know of right now, but I think it's better just to use this one pattern everywhere. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55600/#review162882 --- On Jan. 17, 2017, 6:41 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55600/ > --- > > (Updated Jan. 17, 2017, 6:41 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Stout currently manually groups its source code by folder in order to > present the source nicely in IDEs like XCode and Visual Studio. > > With the recently-introduced CMake function, `GROUP_SOURCE`, this > process is automated away. This commit will remove these manual groups > and replace it with a call to this function. > > > Diffs > - > > 3rdparty/stout/cmake/StoutConfigure.cmake > bc27ac687bae4e1798eece562027ba33c6b32348 > 3rdparty/stout/tests/CMakeLists.txt > a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae > > Diff: https://reviews.apache.org/r/55600/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55549: Windows: Added health checker to build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55549/ --- (Updated Jan. 26, 2017, 8:45 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's comments. Bugs: MESOS-6709 https://issues.apache.org/jira/browse/MESOS-6709 Repository: mesos Description --- This is a blocker for MESOS-6709, which requires both the health checker and the TCP connector to build and work on Windows. Diffs (updated) - src/checks/CMakeLists.txt ce195e2820c70dd7ebec1f06a6382f58fc729af7 src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5 src/checks/tcp_connect.cpp ad1e932da53c8f6b0ae77dfa6b6bb3d642273af9 Diff: https://reviews.apache.org/r/55549/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55549: Windows: Added health checker to build.
> On Jan. 24, 2017, 9:37 p.m., Joseph Wu wrote: > > src/health-check/tcp_connect.cpp, lines 33-38 > > <https://reviews.apache.org/r/55549/diff/1/?file=1605582#file1605582line33> > > > > Libprocess headers should go above stout headers. Oh, did this change recently, or have I just always done this wrong? > On Jan. 24, 2017, 9:37 p.m., Joseph Wu wrote: > > src/health-check/tcp_connect.cpp, lines 148-150 > > <https://reviews.apache.org/r/55549/diff/1/?file=1605582#file1605582line148> > > > > Should there be a `process::finalize` here? Actually, now that I think about it, this should not take a dependency on libprocess at all. Let's use the Stout WSA functions instead. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55549/#review162853 ------- On Jan. 15, 2017, 9:35 p.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55549/ > --- > > (Updated Jan. 15, 2017, 9:35 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Bugs: MESOS-6709 > https://issues.apache.org/jira/browse/MESOS-6709 > > > Repository: mesos > > > Description > --- > > This is a blocker for MESOS-6709, which requires both the health checker > and the TCP connector to build and work on Windows. > > > Diffs > - > > src/health-check/CMakeLists.txt ce195e2820c70dd7ebec1f06a6382f58fc729af7 > src/health-check/health_checker.cpp > a8424b75927d15dc1b897faf0e47cf075c70ff26 > src/health-check/tcp_connect.cpp ad1e932da53c8f6b0ae77dfa6b6bb3d642273af9 > > Diff: https://reviews.apache.org/r/55549/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.
> On Jan. 21, 2017, 12:30 a.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/launch.cpp, line 53 > > <https://reviews.apache.org/r/55037/diff/7/?file=1608655#file1608655line53> > > > > This looks like an erroneous addition. This doesn't appear in my source file, and when I push it doesn't go away. I think it must be a RB error. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55037/#review162525 --- On Jan. 19, 2017, 2:13 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55037/ > --- > > (Updated Jan. 19, 2017, 2:13 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, > and Joseph Wu. > > > Bugs: MESOS-6839 > https://issues.apache.org/jira/browse/MESOS-6839 > > > Repository: mesos > > > Description > --- > > Currently, when the containerizer launches a process with the POSIX > launcher, it will check if the `$PATH` environment variable (or `%PATH%` > in the case of Windows) is set in the `LaunchInfo`. If it is not, we > supply a default path. Unfortunately, this default path is specific to > POSIX. In many of our tests, this causes many of our tests to be unable > to find Windows-standard executables like `ping`, and subsequently fail. > > This commit will introduce a function, `defaultPath` that returns a > sensible default path for both POSIX and Windows. Since the Windows > implementation of this depends on the configuration of the host running > the containerizer (rather than, say, the one creating the `TaskInfo`), > we choose to implement this in `launch.cpp` instead of Stout, where a > user could mistakenly call it and expect the same output on all hosts. > > > Diffs > - > > src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > ea91c71fdfac48a2fc1d31a0ee088a73244be367 > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp > ab4b88acddc7503e16e3730320df39a2f104539a > src/slave/containerizer/mesos/launch.cpp > e482ab8bdfc358f695b87cda72ca59fb64cd8c4d > > Diff: https://reviews.apache.org/r/55037/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Review Request 55749: Added CMake to standard documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55749/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3139 https://issues.apache.org/jira/browse/MESOS-3139 Repository: mesos Description --- Added CMake to standard documentation. Diffs - docs/configuration.md 2113d06f58dddc0a28ae1241a24096266fe39801 Diff: https://reviews.apache.org/r/55749/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55748: CMake: Deleted spurious configuration settings in agent and master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55748/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- CMake: Deleted spurious configuration settings in agent and master. Diffs - src/master/cmake/MasterConfigure.cmake 3d316d6ff2910fc360b0faecb5e6ac9687a77883 src/slave/cmake/AgentConfigure.cmake 1582127ccce773af6031a5b09252192b05a13cdc Diff: https://reviews.apache.org/r/55748/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55037/ --- (Updated Jan. 19, 2017, 2:13 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, and Joseph Wu. Bugs: MESOS-6839 https://issues.apache.org/jira/browse/MESOS-6839 Repository: mesos Description --- Currently, when the containerizer launches a process with the POSIX launcher, it will check if the `$PATH` environment variable (or `%PATH%` in the case of Windows) is set in the `LaunchInfo`. If it is not, we supply a default path. Unfortunately, this default path is specific to POSIX. In many of our tests, this causes many of our tests to be unable to find Windows-standard executables like `ping`, and subsequently fail. This commit will introduce a function, `defaultPath` that returns a sensible default path for both POSIX and Windows. Since the Windows implementation of this depends on the configuration of the host running the containerizer (rather than, say, the one creating the `TaskInfo`), we choose to implement this in `launch.cpp` instead of Stout, where a user could mistakenly call it and expect the same output on all hosts. Diffs (updated) - src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ea91c71fdfac48a2fc1d31a0ee088a73244be367 src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp ab4b88acddc7503e16e3730320df39a2f104539a src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d Diff: https://reviews.apache.org/r/55037/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55037/ --- (Updated Jan. 19, 2017, 2:09 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, and Joseph Wu. Bugs: MESOS-6839 https://issues.apache.org/jira/browse/MESOS-6839 Repository: mesos Description --- Currently, when the containerizer launches a process with the POSIX launcher, it will check if the `$PATH` environment variable (or `%PATH%` in the case of Windows) is set in the `LaunchInfo`. If it is not, we supply a default path. Unfortunately, this default path is specific to POSIX. In many of our tests, this causes many of our tests to be unable to find Windows-standard executables like `ping`, and subsequently fail. This commit will introduce a function, `defaultPath` that returns a sensible default path for both POSIX and Windows. Since the Windows implementation of this depends on the configuration of the host running the containerizer (rather than, say, the one creating the `TaskInfo`), we choose to implement this in `launch.cpp` instead of Stout, where a user could mistakenly call it and expect the same output on all hosts. Diffs (updated) - src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ea91c71fdfac48a2fc1d31a0ee088a73244be367 src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp ab4b88acddc7503e16e3730320df39a2f104539a src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d src/slave/containerizer/mesos/utils.hpp a54106dc4893bb222f42ede936ac9029e817faf9 Diff: https://reviews.apache.org/r/55037/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55037/ --- (Updated Jan. 19, 2017, 2:06 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, and Joseph Wu. Changes --- Addressed Joseph's comments. Bugs: MESOS-6839 https://issues.apache.org/jira/browse/MESOS-6839 Repository: mesos Description --- Currently, when the containerizer launches a process with the POSIX launcher, it will check if the `$PATH` environment variable (or `%PATH%` in the case of Windows) is set in the `LaunchInfo`. If it is not, we supply a default path. Unfortunately, this default path is specific to POSIX. In many of our tests, this causes many of our tests to be unable to find Windows-standard executables like `ping`, and subsequently fail. This commit will introduce a function, `defaultPath` that returns a sensible default path for both POSIX and Windows. Since the Windows implementation of this depends on the configuration of the host running the containerizer (rather than, say, the one creating the `TaskInfo`), we choose to implement this in `launch.cpp` instead of Stout, where a user could mistakenly call it and expect the same output on all hosts. Diffs (updated) - src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ea91c71fdfac48a2fc1d31a0ee088a73244be367 src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp ab4b88acddc7503e16e3730320df39a2f104539a src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d src/slave/containerizer/mesos/utils.hpp a54106dc4893bb222f42ede936ac9029e817faf9 Diff: https://reviews.apache.org/r/55037/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55699: Stout: Added `host_default_path`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55699/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- Stout: Added `host_default_path`. Diffs - 3rdparty/stout/include/stout/posix/os.hpp 397c2d6b06ddb607d254eae8258da5151c5f57e0 3rdparty/stout/include/stout/windows/os.hpp b4a66ba8ddbc60f7bcc9aad7fef4e158ae3a96d6 Diff: https://reviews.apache.org/r/55699/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55694: CMake: Separated Stout system headers from Stout API headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55694/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3932 https://issues.apache.org/jira/browse/MESOS-3932 Repository: mesos Description --- MESOS-3932 specifies a bug that causes spurious warnings to be printed when building Mesos. Many of these warnings can be eliminated simply by correctly marking off the third-party dependencies as `SYSTEM` headers when we specify the include path. This commit will thus separate out the headers of Stout's third-party dependencies from the core Stout API headers, and include them as `SYSTEM` headers. Diffs - 3rdparty/stout/cmake/StoutConfigure.cmake bc27ac687bae4e1798eece562027ba33c6b32348 3rdparty/stout/cmake/StoutTestsConfigure.cmake d3bd72e8eba77213095da6cabb3a6d6f4d30941c 3rdparty/stout/tests/CMakeLists.txt a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae Diff: https://reviews.apache.org/r/55694/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55695: CMake: Separated Libprocess system headers from Libprocess API headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55695/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3932 https://issues.apache.org/jira/browse/MESOS-3932 Repository: mesos Description --- MESOS-3932 specifies a bug that causes spurious warnings to be printed when building Mesos. Many of these warnings can be eliminated simply by correctly marking off the third-party dependencies as `SYSTEM` headers when we specify the include path. This commit will thus separate out the headers of Libprocesses's third-party dependencies from the core Libprocess API headers, and include them as `SYSTEM` headers. Notably, we choose to include Stout's headers as third-party system headers, since the goal is for them to be completely independent projects. Diffs - 3rdparty/libprocess/cmake/ProcessConfigure.cmake 873f41d844faa0d442f77411e94314a89be5f046 3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 49ad836d5fa3f84cdf5ae0e08f449cd7ef2537a1 3rdparty/libprocess/src/CMakeLists.txt 60f0e76dfd237d9a12a366b413802d1a96892b55 Diff: https://reviews.apache.org/r/55695/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55696: CMake: Separated Mesos system headers from Mesos API headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55696/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3932 https://issues.apache.org/jira/browse/MESOS-3932 Repository: mesos Description --- MESOS-3932 specifies a bug that causes spurious warnings to be printed when building Mesos. Many of these warnings can be eliminated simply by correctly marking off the third-party dependencies as `SYSTEM` headers when we specify the include path. This commit will thus separate out the headers of Mesos's third-party dependencies from the core Mesos API headers, and include them as `SYSTEM` headers. Notably, we choose to include Stout's and Libprocess's headers as third-party system headers, since the goal is for them to be completely independent projects. Diffs - src/CMakeLists.txt ce71afc73f85a70cd8f97a6e913662ff7ef0d94c src/docker/CMakeLists.txt 892370d603d42b649afbf05deece5e0b1d97ee98 src/master/CMakeLists.txt a9fb34fc7099f05e279ee0506dd18bc0745d546c src/master/cmake/MasterConfigure.cmake 3d316d6ff2910fc360b0faecb5e6ac9687a77883 src/slave/CMakeLists.txt 03466bda934290ce41fa6408d35ccae10c9a6f32 src/slave/cmake/AgentConfigure.cmake 1582127ccce773af6031a5b09252192b05a13cdc src/tests/CMakeLists.txt b7adf4f3126611a57baf4f62b62b208bc0236da5 src/tests/cmake/MesosTestsConfigure.cmake 8d416388a8e45a2832ae3841b58541ba5b0613bc Diff: https://reviews.apache.org/r/55696/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55637: CMake: Added `test` target.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55637/ --- (Updated Jan. 18, 2017, 11:34 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Addressed Ilya's very good point. Bugs: MESOS-3697 https://issues.apache.org/jira/browse/MESOS-3697 Repository: mesos Description --- This commit will introduce a `test` target to the CMake build system. The semantics of this target are very similar to the autotools build system, in that they build the tests, but do not run them. Accomplishing this is somewhat complex in CMake, because CMake expects to control the `test` target itself. We work around this by (1) silencing the warning that `test` is a reserved target, (2) removing the call to `enable_testing` that sets up CTest, and (3) adding our own `test` target. As an additional insurance policy, we error out if any of the `BUILD_TESTING` flags are defined, which would indicate that `include(CTest)` was called. Diffs (updated) - CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f Diff: https://reviews.apache.org/r/55637/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55637: CMake: Added `test` target.
> On Jan. 17, 2017, 8:46 p.m., Ilya Pronin wrote:
> > I think the name "test" can confuse people because it's usually expected to
> > be used for CTest. Maybe it would better be leave old {{tests}} and
> > {{check}} targets? https://cmake.org/Wiki/CMakeEmulateMakeCheck
>
> Alex Clemmer wrote:
> That's a great question. I'm not a CMake native, so it's not clear to me
> how weird this would be to do. The idea was to expose a `test` target to
> achieve something resembling script parity. Is this idea not compelling to
> you?
>
> Ilya Pronin wrote:
> Very compelling. I just meant that `make test` is usually expected to not
> only build tests but also run them, i.e. act the same way as autotools
> generated `make check`. E.g. debhelper's `dh_auto_test` looks for `test` or
> `check` target in a Makefile. So I suggested that target that just builds
> tests should be called `tests`.
>
> Alex Clemmer wrote:
> Ah, ok, let me just re-state what I think is true to make sure we're on
> the page. Right now the autotools build has the semantics that `make check`
> will build + run tests, and `make test` will simply build them. So the
> original idea here was that `make test` achieve script parity for our own
> tooling. And it sounds like what you're saying is that this should actually
> be the goal -- the goal should be that we do something more congruent with
> the normal use of the CMake semantics of `make test` and then have another
> target (which could be called, _e.g._, `make tests`, with an `s`) that
> implements these semantics.
>
> Is that all approximately correct?
>
> If so, let's wait and see what `kaysoky` says. He has a better
> understanding for the impact of changing the semantics of the `make test`
> target. IMHO this is a sensible suggestion, but it's important to get input
> for how much work this creates for the downstream consumers of the `make
> test` target.
>
> Ilya Pronin wrote:
> Yes, except for 1 thing: current autotools target that builds tests is
> called `tests` :)
> https://github.com/apache/mesos/blob/master/src/Makefile.am#L2490
Oh, oh, oh. Got it. In my defense, I don't know what the hell I'm doing. :)
Let's discard the reviews that disable CTest and re-write this to define the
target as `tests`.
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55637/#review161940
---
On Jan. 17, 2017, 7:33 p.m., Alex Clemmer wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55637/
> ---
>
> (Updated Jan. 17, 2017, 7:33 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-3697
> https://issues.apache.org/jira/browse/MESOS-3697
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This commit will introduce a `test` target to the CMake build system.
> The semantics of this target are very similar to the autotools build
> system, in that they build the tests, but do not run them.
>
> Accomplishing this is somewhat complex in CMake, because CMake expects
> to control the `test` target itself. We work around this by (1)
> silencing the warning that `test` is a reserved target, (2) removing the
> call to `enable_testing` that sets up CTest, and (3) adding our own
> `test` target. As an additional insurance policy, we error out if any of
> the `BUILD_TESTING` flags are defined, which would indicate that
> `include(CTest)` was called.
>
>
> Diffs
> -
>
> CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f
> cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc
> src/tests/CMakeLists.txt b7adf4f3126611a57baf4f62b62b208bc0236da5
>
> Diff: https://reviews.apache.org/r/55637/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55313: Windows: Fixed the unkillable task bug, lit up executor tests.
> On Jan. 18, 2017, 11:04 p.m., Joseph Wu wrote: > > src/tests/default_executor_tests.cpp, lines 442-444 > > <https://reviews.apache.org/r/55313/diff/2/?file=1608174#file1608174line442> > > > > Here too. Oh, shoot, sorry. I should have caught that. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55313/#review162200 --- On Jan. 18, 2017, 8:49 p.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55313/ > --- > > (Updated Jan. 18, 2017, 8:49 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Bugs: MESOS-6698, MESOS-6839 and MESOS-6870 > https://issues.apache.org/jira/browse/MESOS-6698 > https://issues.apache.org/jira/browse/MESOS-6839 > https://issues.apache.org/jira/browse/MESOS-6870 > > > Repository: mesos > > > Description > --- > > MESOS-6839 tracks a bug that causes the current implementation of the > default executor to be unable to delete any processes associated with a > task. To understand why requires some knowledge of the differences > between the process model of Windows and Unix. > > In Unix, there is a robust notion of a process tree, with a well-defined > notion of process groups, sessions, signal delivery on the tree, and so > on. Windows lacks a robust notion of a process hierarchy, and therefore > largely has no equivalents to these constructs (including, notably, > signal semantics). > > One of the problems this mismatch causes Mesos is that it complicates > the problem of killing a task, which is at its core a group of > processes. On Windows, the easiest way to make a process and all its > descendents easily killable is to track these processes in a Job Object, > which is a Windows kernel construct similar in principle to Linux's > control groups (though with different ideas of process namespacing). > > There is some subtlety in making sure _all_ processes associated with a > task are captured inside a Job Object. The most important consideration > is that we need to make sure to add any process to the Job Object before > it has a chance to create any child processes; if we fail to do this, > the children will not be captured in the Job Object. > > The solution to this is fairly simple on Windows. The process creation > API allows users to trivially create a process in a suspended state, so > that the Windows kernel scheduler does not schedule the process to run > until the user explicitly resumes the main thread. This allows us to > create the process and add it to a Job Object before it has a chance to > create children, and then start the process. > > This commit will accomplish this by changing `PosixLauncher::fork` to > use the Subprocess parent hooks API, which implements exactly this > semantics. Essentially, the launcher will launch the containerizer > process, which will inspect the TaskInfo or the environment for a task > to launch, and then launch it. Using the parent hooks API, Subprocess > will create the containerizer process on Windows in a suspended state, > and then the parent hook supplied by the launcher will add that process > to a Job Object before it has a chance to run. Finally, Subprocess will > mark the process as runnable, and return. > > This commit resolves MESOS-6839. We also light up the executor tests, so > it also resolves MESOS-6870 and MESOS-6839. > > > Diffs > - > > src/slave/containerizer/mesos/launcher.cpp > a6a8c01cb39f35f8174fcb5af0ef18de2da5ee78 > src/tests/command_executor_tests.cpp > 4d5c21ec427ebaac053e56ae554cb466dfeb0b8b > src/tests/default_executor_tests.cpp > ec3e854ed58a0fbb3bfad0bd21eb0e2974548865 > > Diff: https://reviews.apache.org/r/55313/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55311: Added `process::initialize` to default executor's `main`.
> On Jan. 18, 2017, 1:50 a.m., Joseph Wu wrote: > > src/launcher/default_executor.cpp, line 1055 > > <https://reviews.apache.org/r/55311/diff/1/?file=1599651#file1599651line1055> > > > > It actually feels like `process::Winsock winsock;` is more appropriate > > here, as we want the socket stack to be cleaned up afterwards as well. > > Either than, or you should add a `process::finalize(true)` befow. > > Alex Clemmer wrote: > Tearing down winsock before libprocess is terminated will cause ugly > failures as the test framework disposes itself. Let's do a call to > `process::finalize(true)`. This might not have been clear, btw, so let me be more specific: if we use the `Winsock` class, we will shut down the winsock stack when we exit `main`. But, at that point, libprocess will typically still be running, and it is therefore highly likely that it will start throwing errors as (_e.g._) its open sockets start to recieve errors. The desired dispose path is for libprocess to dispose of all of its assets, _and then_ dispose of the winsock stack last. This is the semantics of `process::finalize(true)`, which is why we should use it here and not the `Winsock` class. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55311/#review162013 ------- On Jan. 18, 2017, 6:37 p.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55311/ > --- > > (Updated Jan. 18, 2017, 6:37 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The default executor's `main` currently depends on `UPID`, which > networking libraries to do things like retrieve the address of the > current host. On Windows, this means that it is required to initialize > the winsock stack to be initialized before `UPID` is used. > > To resolve this issue, we add a call to `process::initialize` at the > beginning of the `main`, which will cause the stack to be initialized > correctly. > > > Diffs > - > > src/launcher/default_executor.cpp 57e4799e750f8f5352a9fec58af40efe432ea865 > > Diff: https://reviews.apache.org/r/55311/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55024/ --- (Updated Jan. 18, 2017, 9:55 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, and Joseph Wu. Changes --- Address Joseph's last comment. Repository: mesos Description --- Currently libprocess will attempt to use sockets without initializing the socket stack on Windows. This commit will resolving this problem by causing `process::initialize` to initialize the socket stack. Diffs (updated) - 3rdparty/libprocess/include/process/process.hpp b118f1a2bf5aac12b53ae204253b88c9b1c65f46 3rdparty/libprocess/src/process.cpp f475fe78f801924f70f51fdc4ab190c2dbecd656 Diff: https://reviews.apache.org/r/55024/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.
> On Jan. 18, 2017, 1:06 a.m., Joseph Wu wrote: > > 3rdparty/libprocess/src/process.cpp, line 1328 > > <https://reviews.apache.org/r/55024/diff/2/?file=1605541#file1605541line1328> > > > > At the moment, I would not expect this to ever fail, > > `EXIT(EXIT_FAILURE)` is preferable. If it does, it seems like a severe > > enough error (as the docs suggest most errors are due to programmer error). > > > > See: > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms741549(v=vs.85).aspx > > Alex Clemmer wrote: > The reason I decided to `LOG(ERROR)` is because there didn't seem to be > any scenarios where we would encounter errors because we haven't shut down > the socket stack. We can change it if you feel strongly about it, though. Do > you think there is a strong possibility of an error condition that I'm > missing? > > Joseph Wu wrote: > We generally err on the side of failing-fast. So if we don't expect > something to occur, we first place a `CHECK` or `EXIT`. If that turns out to > be incorrect later, we consider relaxing the code at that point. > > This usually gives more visibility into problems, as people are more > likely to notice a crash than an error log. I agree about fail-fast, which is good distributed systems hygiene. It just seems to me that the dispose path is basically never the cause of service errors. But, anyway, let's change it. I just wanted to double-check there wasn't a specific error case I was missing. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55024/#review162008 --- On Jan. 18, 2017, 5:41 p.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55024/ > --- > > (Updated Jan. 18, 2017, 5:41 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Currently libprocess will attempt to use sockets without initializing > the socket stack on Windows. This commit will resolving this problem by > causing `process::initialize` to initialize the socket stack. > > > Diffs > - > > 3rdparty/libprocess/include/process/process.hpp > b118f1a2bf5aac12b53ae204253b88c9b1c65f46 > 3rdparty/libprocess/src/process.cpp > f475fe78f801924f70f51fdc4ab190c2dbecd656 > > Diff: https://reviews.apache.org/r/55024/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55327: Windows: Fixed hanging symlink bug in `os::rmdir`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55327/ --- (Updated Jan. 18, 2017, 8:50 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's comments, fix embarrasing mistake. Repository: mesos Description --- The Windows implementation of `os::rmdir` will fail to delete "hanging" symlinks (i.e., symlinks whose targets do not exist). Note that on Windows this bug is specific to symlinks whose targets are _deleted_, since it is impossible to create a symlink whose target does not exist. The primary issue that causes this problem is that it is very difficult to tell whether a symlink points at a directory or a file unless you resolve the symlink and determine whether the target is a directory or a file. In situations where the target does not exist, we can't use this information, and so `os::rmdir` occasionally mis-routes a symlink to (what was) a directory to a `::remove` call, which will fail with a cryptic error. To fix this behavior, this commit will introduce code that simply tries to remove the reparse point with both `RemoveDirectory` and `DeleteFile`, and if either succeeds, we report success for the operation. This represents a "best effort"; in the case that the reparse point represents something more exotic than a symlink, we will still fail, but by choosing not to verify whether the target is a directory or a file, we simplify the code and still obtain the outcome of having deleted the directory. This commit is the primary blocker for MESOS-6707, as deleting the Agent sandbox will sometimes cause us to delete the latest run directory for the executor before the symlinked `latest` directory itself. This causes the delete to fail, and then the GC tests to fail, since they tend to assert the directory does not exist. Diffs (updated) - 3rdparty/stout/include/stout/os/windows/rmdir.hpp 4437484c068e9ef046e0be14683c97db447f2da1 3rdparty/stout/tests/os/rmdir_tests.cpp 988d41b7fdd11cc96ce005671a7c62d1b5a3615d Diff: https://reviews.apache.org/r/55327/diff/ Testing ------- Thanks, Alex Clemmer
Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55023/ --- (Updated Jan. 18, 2017, 8:49 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, and Joseph Wu. Changes --- Address Joseph's comments. Repository: mesos Description --- Currently in `MesosContainerizerProcess::_launch`, we are passing a malformatted shell command to the launcher. This causes the containerizer process to crash immediately upon invocation in all executor tests. This commit will fix this command. Diffs (updated) - src/slave/containerizer/mesos/containerizer.cpp 8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 Diff: https://reviews.apache.org/r/55023/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55313: Windows: Fixed the unkillable task bug, lit up executor tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55313/ --- (Updated Jan. 18, 2017, 8:49 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's comments. Bugs: MESOS-6698, MESOS-6839 and MESOS-6870 https://issues.apache.org/jira/browse/MESOS-6698 https://issues.apache.org/jira/browse/MESOS-6839 https://issues.apache.org/jira/browse/MESOS-6870 Repository: mesos Description --- MESOS-6839 tracks a bug that causes the current implementation of the default executor to be unable to delete any processes associated with a task. To understand why requires some knowledge of the differences between the process model of Windows and Unix. In Unix, there is a robust notion of a process tree, with a well-defined notion of process groups, sessions, signal delivery on the tree, and so on. Windows lacks a robust notion of a process hierarchy, and therefore largely has no equivalents to these constructs (including, notably, signal semantics). One of the problems this mismatch causes Mesos is that it complicates the problem of killing a task, which is at its core a group of processes. On Windows, the easiest way to make a process and all its descendents easily killable is to track these processes in a Job Object, which is a Windows kernel construct similar in principle to Linux's control groups (though with different ideas of process namespacing). There is some subtlety in making sure _all_ processes associated with a task are captured inside a Job Object. The most important consideration is that we need to make sure to add any process to the Job Object before it has a chance to create any child processes; if we fail to do this, the children will not be captured in the Job Object. The solution to this is fairly simple on Windows. The process creation API allows users to trivially create a process in a suspended state, so that the Windows kernel scheduler does not schedule the process to run until the user explicitly resumes the main thread. This allows us to create the process and add it to a Job Object before it has a chance to create children, and then start the process. This commit will accomplish this by changing `PosixLauncher::fork` to use the Subprocess parent hooks API, which implements exactly this semantics. Essentially, the launcher will launch the containerizer process, which will inspect the TaskInfo or the environment for a task to launch, and then launch it. Using the parent hooks API, Subprocess will create the containerizer process on Windows in a suspended state, and then the parent hook supplied by the launcher will add that process to a Job Object before it has a chance to run. Finally, Subprocess will mark the process as runnable, and return. This commit resolves MESOS-6839. We also light up the executor tests, so it also resolves MESOS-6870 and MESOS-6839. Diffs (updated) - src/slave/containerizer/mesos/launcher.cpp a6a8c01cb39f35f8174fcb5af0ef18de2da5ee78 src/tests/command_executor_tests.cpp 4d5c21ec427ebaac053e56ae554cb466dfeb0b8b src/tests/default_executor_tests.cpp ec3e854ed58a0fbb3bfad0bd21eb0e2974548865 Diff: https://reviews.apache.org/r/55313/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55312: Windows: Added parent hooks to subprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55312/ --- (Updated Jan. 18, 2017, 8:49 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's comments from r/55313. Repository: mesos Description --- The subprocess API exposes various hooks to customize behavior as we create the child process. Currently, these hooks are unimplemented in the Windows codepaths. This commit will implement the parent hooks -- which are executed after the child process is created, but before it begins execution -- on the Windows codepath. Diffs (updated) - 3rdparty/libprocess/include/process/subprocess_base.hpp 74a4bef0706334b4d3c1040a08a8921fbc300344 3rdparty/libprocess/include/process/windows/subprocess.hpp 3bc7f1992d9c38dac2ec23d5bc57415f37d0318a 3rdparty/libprocess/src/subprocess.cpp ad19b0896b4a2e9c60f573cc854c10c69e909e86 Diff: https://reviews.apache.org/r/55312/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55637: CMake: Added `test` target.
> On Jan. 17, 2017, 8:46 p.m., Ilya Pronin wrote:
> > I think the name "test" can confuse people because it's usually expected to
> > be used for CTest. Maybe it would better be leave old {{tests}} and
> > {{check}} targets? https://cmake.org/Wiki/CMakeEmulateMakeCheck
>
> Alex Clemmer wrote:
> That's a great question. I'm not a CMake native, so it's not clear to me
> how weird this would be to do. The idea was to expose a `test` target to
> achieve something resembling script parity. Is this idea not compelling to
> you?
>
> Ilya Pronin wrote:
> Very compelling. I just meant that `make test` is usually expected to not
> only build tests but also run them, i.e. act the same way as autotools
> generated `make check`. E.g. debhelper's `dh_auto_test` looks for `test` or
> `check` target in a Makefile. So I suggested that target that just builds
> tests should be called `tests`.
Ah, ok, let me just re-state what I think is true to make sure we're on the
page. Right now the autotools build has the semantics that `make check` will
build + run tests, and `make test` will simply build them. So the original idea
here was that `make test` achieve script parity for our own tooling. And it
sounds like what you're saying is that this should actually be the goal -- the
goal should be that we do something more congruent with the normal use of the
CMake semantics of `make test` and then have another target (which could be
called, _e.g._, `make tests`, with an `s`) that implements these semantics.
Is that all approximately correct?
If so, let's wait and see what `kaysoky` says. He has a better understanding
for the impact of changing the semantics of the `make test` target. IMHO this
is a sensible suggestion, but it's important to get input for how much work
this creates for the downstream consumers of the `make test` target.
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55637/#review161940
-------
On Jan. 17, 2017, 7:33 p.m., Alex Clemmer wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55637/
> ---
>
> (Updated Jan. 17, 2017, 7:33 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-3697
> https://issues.apache.org/jira/browse/MESOS-3697
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This commit will introduce a `test` target to the CMake build system.
> The semantics of this target are very similar to the autotools build
> system, in that they build the tests, but do not run them.
>
> Accomplishing this is somewhat complex in CMake, because CMake expects
> to control the `test` target itself. We work around this by (1)
> silencing the warning that `test` is a reserved target, (2) removing the
> call to `enable_testing` that sets up CTest, and (3) adding our own
> `test` target. As an additional insurance policy, we error out if any of
> the `BUILD_TESTING` flags are defined, which would indicate that
> `include(CTest)` was called.
>
>
> Diffs
> -
>
> CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f
> cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc
> src/tests/CMakeLists.txt b7adf4f3126611a57baf4f62b62b208bc0236da5
>
> Diff: https://reviews.apache.org/r/55637/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55327: Windows: Fixed hanging symlink bug in `os::rmdir`.
> On Jan. 18, 2017, 2:32 a.m., Joseph Wu wrote: > > 3rdparty/stout/include/stout/os/windows/rmdir.hpp, lines 120-123 > > <https://reviews.apache.org/r/55327/diff/2/?file=1605538#file1605538line120> > > > > Note: This is actually unreachable. :| Embarrassing. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55327/#review162019 --- On Jan. 15, 2017, 8:40 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55327/ > --- > > (Updated Jan. 15, 2017, 8:40 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The Windows implementation of `os::rmdir` will fail to delete "hanging" > symlinks (i.e., symlinks whose targets do not exist). Note that on > Windows this bug is specific to symlinks whose targets are _deleted_, > since it is impossible to create a symlink whose target does not exist. > > The primary issue that causes this problem is that it is very difficult > to tell whether a symlink points at a directory or a file unless you > resolve the symlink and determine whether the target is a directory or a > file. In situations where the target does not exist, we can't use this > information, and so `os::rmdir` occasionally mis-routes a symlink to > (what was) a directory to a `::remove` call, which will fail with a > cryptic error. > > To fix this behavior, this commit will introduce code that simply tries > to remove the reparse point with both `RemoveDirectory` and > `DeleteFile`, and if either succeeds, we report success for the > operation. This represents a "best effort"; in the case that the reparse > point represents something more exotic than a symlink, we will still > fail, but by choosing not to verify whether the target is a directory or > a file, we simplify the code and still obtain the outcome of having > deleted the directory. > > This commit is the primary blocker for MESOS-6707, as deleting the Agent > sandbox will sometimes cause us to delete the latest run directory for > the executor before the symlinked `latest` directory itself. This causes > the delete to fail, and then the GC tests to fail, since they tend to > assert the directory does not exist. > > > Diffs > - > > 3rdparty/stout/include/stout/os/windows/rmdir.hpp > 4437484c068e9ef046e0be14683c97db447f2da1 > 3rdparty/stout/tests/os/rmdir_tests.cpp > 988d41b7fdd11cc96ce005671a7c62d1b5a3615d > > Diff: https://reviews.apache.org/r/55327/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55311: Added `process::initialize` to default executor's `main`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55311/ --- (Updated Jan. 18, 2017, 6:37 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Address Joseph's comments. Repository: mesos Description --- The default executor's `main` currently depends on `UPID`, which networking libraries to do things like retrieve the address of the current host. On Windows, this means that it is required to initialize the winsock stack to be initialized before `UPID` is used. To resolve this issue, we add a call to `process::initialize` at the beginning of the `main`, which will cause the stack to be initialized correctly. Diffs (updated) - src/launcher/default_executor.cpp 57e4799e750f8f5352a9fec58af40efe432ea865 Diff: https://reviews.apache.org/r/55311/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55313: Windows: Fixed the unkillable task bug, lit up executor tests.
> On Jan. 18, 2017, 2:08 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/launcher.cpp, lines 117-133
> > <https://reviews.apache.org/r/55313/diff/1/?file=1599654#file1599654line117>
> >
> > I suspect that we'll use this lambda for other subprocesses in future,
> > so let's move it into `include/process/subprocess_base.hpp` and
> > `src/subprocess.cpp`:
> >
> > ```
> > struct ParentHook
> > {
> > ...
> > #ifdef __WINDOWS__
> > static ParentHook CREATE_JOB();
> > #endif // __WINDOWS__
> > };
> > ```
Ok, we can do this. I do question whether this really belongs as part of the
`Subprocess` API, but I do not will not block that change. :)
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55313/#review162016
---
On Jan. 8, 2017, 6:30 a.m., Alex Clemmer wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55313/
> ---
>
> (Updated Jan. 8, 2017, 6:30 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-6698, MESOS-6839 and MESOS-6870
> https://issues.apache.org/jira/browse/MESOS-6698
> https://issues.apache.org/jira/browse/MESOS-6839
> https://issues.apache.org/jira/browse/MESOS-6870
>
>
> Repository: mesos
>
>
> Description
> ---
>
> MESOS-6839 tracks a bug that causes the current implementation of the
> default executor to be unable to delete any processes associated with a
> task. To understand why requires some knowledge of the differences
> between the process model of Windows and Unix.
>
> In Unix, there is a robust notion of a process tree, with a well-defined
> notion of process groups, sessions, signal delivery on the tree, and so
> on. Windows lacks a robust notion of a process hierarchy, and therefore
> largely has no equivalents to these constructs (including, notably,
> signal semantics).
>
> One of the problems this mismatch causes Mesos is that it complicates
> the problem of killing a task, which is at its core a group of
> processes. On Windows, the easiest way to make a process and all its
> descendents easily killable is to track these processes in a Job Object,
> which is a Windows kernel construct similar in principle to Linux's
> control groups (though with different ideas of process namespacing).
>
> There is some subtlety in making sure _all_ processes associated with a
> task are captured inside a Job Object. The most important consideration
> is that we need to make sure to add any process to the Job Object before
> it has a chance to create any child processes; if we fail to do this,
> the children will not be captured in the Job Object.
>
> The solution to this is fairly simple on Windows. The process creation
> API allows users to trivially create a process in a suspended state, so
> that the Windows kernel scheduler does not schedule the process to run
> until the user explicitly resumes the main thread. This allows us to
> create the process and add it to a Job Object before it has a chance to
> create children, and then start the process.
>
> This commit will accomplish this by changing `PosixLauncher::fork` to
> use the Subprocess parent hooks API, which implements exactly this
> semantics. Essentially, the launcher will launch the containerizer
> process, which will inspect the TaskInfo or the environment for a task
> to launch, and then launch it. Using the parent hooks API, Subprocess
> will create the containerizer process on Windows in a suspended state,
> and then the parent hook supplied by the launcher will add that process
> to a Job Object before it has a chance to run. Finally, Subprocess will
> mark the process as runnable, and return.
>
> This commit resolves MESOS-6839. We also light up the executor tests, so
> it also resolves MESOS-6870 and MESOS-6839.
>
>
> Diffs
> -
>
> src/slave/containerizer/mesos/launcher.cpp
> a6a8c01cb39f35f8174fcb5af0ef18de2da5ee78
> src/tests/command_executor_tests.cpp
> f4e7044b82e8e81d6430551dc0b2a288db10bc3c
> src/tests/default_executor_tests.cpp
> 340e8c8b36a6a3cc6e5bae021e19dfa7a54852c3
>
> Diff: https://reviews.apache.org/r/55313/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55311: Added `process::initialize` to default executor's `main`.
> On Jan. 18, 2017, 1:50 a.m., Joseph Wu wrote: > > src/launcher/default_executor.cpp, line 1055 > > <https://reviews.apache.org/r/55311/diff/1/?file=1599651#file1599651line1055> > > > > It actually feels like `process::Winsock winsock;` is more appropriate > > here, as we want the socket stack to be cleaned up afterwards as well. > > Either than, or you should add a `process::finalize(true)` befow. Tearing down winsock before libprocess is terminated will cause ugly failures as the test framework disposes itself. Let's do a call to `process::finalize(true)`. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55311/#review162013 ------- On Jan. 8, 2017, 6:28 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55311/ > --- > > (Updated Jan. 8, 2017, 6:28 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Repository: mesos > > > Description > --- > > The default executor's `main` currently depends on `UPID`, which > networking libraries to do things like retrieve the address of the > current host. On Windows, this means that it is required to initialize > the winsock stack to be initialized before `UPID` is used. > > To resolve this issue, we add a call to `process::initialize` at the > beginning of the `main`, which will cause the stack to be initialized > correctly. > > > Diffs > - > > src/launcher/default_executor.cpp fd9963996c82461b60888397989e309d51b60aaa > > Diff: https://reviews.apache.org/r/55311/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55024/ --- (Updated Jan. 18, 2017, 5:41 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, and Joseph Wu. Changes --- Address Joseph's comments. Repository: mesos Description --- Currently libprocess will attempt to use sockets without initializing the socket stack on Windows. This commit will resolving this problem by causing `process::initialize` to initialize the socket stack. Diffs (updated) - 3rdparty/libprocess/include/process/process.hpp b118f1a2bf5aac12b53ae204253b88c9b1c65f46 3rdparty/libprocess/src/process.cpp f475fe78f801924f70f51fdc4ab190c2dbecd656 Diff: https://reviews.apache.org/r/55024/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55024: Windows: Start the socket stack in `process::initialize`.
> On Jan. 18, 2017, 1:06 a.m., Joseph Wu wrote: > > 3rdparty/libprocess/src/process.cpp, line 1328 > > <https://reviews.apache.org/r/55024/diff/2/?file=1605541#file1605541line1328> > > > > At the moment, I would not expect this to ever fail, > > `EXIT(EXIT_FAILURE)` is preferable. If it does, it seems like a severe > > enough error (as the docs suggest most errors are due to programmer error). > > > > See: > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms741549(v=vs.85).aspx The reason I decided to `LOG(ERROR)` is because there didn't seem to be any scenarios where we would encounter errors because we haven't shut down the socket stack. We can change it if you feel strongly about it, though. Do you think there is a strong possibility of an error condition that I'm missing? - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55024/#review162008 ------- On Jan. 15, 2017, 8:46 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55024/ > --- > > (Updated Jan. 15, 2017, 8:46 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Currently libprocess will attempt to use sockets without initializing > the socket stack on Windows. This commit will resolving this problem by > causing `process::initialize` to initialize the socket stack. > > > Diffs > - > > 3rdparty/libprocess/include/process/process.hpp > b118f1a2bf5aac12b53ae204253b88c9b1c65f46 > 3rdparty/libprocess/src/process.cpp > f475fe78f801924f70f51fdc4ab190c2dbecd656 > > Diff: https://reviews.apache.org/r/55024/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.
> On Jan. 18, 2017, 12:47 a.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/containerizer.cpp, lines 1608-1617 > > <https://reviews.apache.org/r/55023/diff/2/?file=1605568#file1605568line1608> > > > > The effect of this is to change `argv` from: > > > > ``` > > mesos-containerizer launch > > ``` > > > > To: > > > > ``` > > .../mesos/libexec/mesos-containerizer launch > > ``` > > > > This change is harmless on POSIX, so there's no need for ifdef-ing. > > > > If so, can't you accomplish the same thing with a diff like: > > ``` > > diff --git a/src/slave/containerizer/mesos/containerizer.cpp > > b/src/slave/containerizer/mesos/containerizer.cpp > > index 8bf8a77..c760130 100644 > > --- a/src/slave/containerizer/mesos/containerizer.cpp > > +++ b/src/slave/containerizer/mesos/containerizer.cpp > > @@ -1603,12 +1603,12 @@ Future MesosContainerizerProcess::_launch( > > > >// Fork the child using launcher. > >vector argv(2); > > - argv[0] = MESOS_CONTAINERIZER; > > + argv[0] = path::join(flags.launcher_dir, MESOS_CONTAINERIZER); > >argv[1] = MesosContainerizerLaunch::NAME; > > > >Try forked = launcher->fork( > >containerId, > > - path::join(flags.launcher_dir, MESOS_CONTAINERIZER), > > + argv[0], > >argv, > >in.isSome() ? in.get() : Subprocess::FD(STDIN_FILENO), > >out.isSome() ? out.get() : Subprocess::FD(STDOUT_FILENO), > > ``` We've historically been super conservative about changing the POSIX path, but with your sign-off, I'm happy to do something like this. > On Jan. 18, 2017, 12:47 a.m., Joseph Wu wrote: > > src/slave/containerizer/mesos/containerizer.cpp, lines 1608-1609 > > <https://reviews.apache.org/r/55023/diff/2/?file=1605568#file1605568line1608> > > > > It would appear that this TODO no longer applies. This issue is not yet resolved, actually. The point I was trying to get at is that the first argument to `subprocess` (if memory serves) is a no-op. So this call is likely to change when we refactor `subprocess`. We can still delete it if you want, though, but I think it's reasonable to keep it. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55023/#review162001 --- On Jan. 15, 2017, 10:46 a.m., Alex Clemmer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55023/ > --- > > (Updated Jan. 15, 2017, 10:46 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, > and Joseph Wu. > > > Repository: mesos > > > Description > --- > > Currently in `MesosContainerizerProcess::_launch`, we are passing a > malformatted shell command to the launcher. This causes the > containerizer process to crash immediately upon invocation in all > executor tests. > > This commit will fix this command. > > > Diffs > - > > src/slave/containerizer/mesos/containerizer.cpp > 8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 > > Diff: https://reviews.apache.org/r/55023/diff/ > > > Testing > --- > > > Thanks, > > Alex Clemmer > >
Re: Review Request 55637: CMake: Added `test` target.
> On Jan. 17, 2017, 8:46 p.m., Ilya Pronin wrote:
> > I think the name "test" can confuse people because it's usually expected to
> > be used for CTest. Maybe it would better be leave old {{tests}} and
> > {{check}} targets? https://cmake.org/Wiki/CMakeEmulateMakeCheck
That's a great question. I'm not a CMake native, so it's not clear to me how
weird this would be to do. The idea was to expose a `test` target to achieve
something resembling script parity. Is this idea not compelling to you?
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55637/#review161940
---
On Jan. 17, 2017, 7:33 p.m., Alex Clemmer wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55637/
> ---
>
> (Updated Jan. 17, 2017, 7:33 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Bugs: MESOS-3697
> https://issues.apache.org/jira/browse/MESOS-3697
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This commit will introduce a `test` target to the CMake build system.
> The semantics of this target are very similar to the autotools build
> system, in that they build the tests, but do not run them.
>
> Accomplishing this is somewhat complex in CMake, because CMake expects
> to control the `test` target itself. We work around this by (1)
> silencing the warning that `test` is a reserved target, (2) removing the
> call to `enable_testing` that sets up CTest, and (3) adding our own
> `test` target. As an additional insurance policy, we error out if any of
> the `BUILD_TESTING` flags are defined, which would indicate that
> `include(CTest)` was called.
>
>
> Diffs
> -
>
> CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f
> cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc
> src/tests/CMakeLists.txt b7adf4f3126611a57baf4f62b62b208bc0236da5
>
> Diff: https://reviews.apache.org/r/55637/diff/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Alex Clemmer
>
>
Review Request 55636: Libprocess: Removed usage of CTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55636/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3697 https://issues.apache.org/jira/browse/MESOS-3697 Repository: mesos Description --- CTest automatically defines a `test` target, which directly impedes the resolution of MESOS-3697. Since Libprocess doesn't make use of CTest anyway, this commit will simply retire it from Libprocess. Diffs - 3rdparty/libprocess/src/tests/CMakeLists.txt 0b2660cb16f5d8d8dc66e6995061d0b832182351 Diff: https://reviews.apache.org/r/55636/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55637: CMake: Added `test` target.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55637/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3697 https://issues.apache.org/jira/browse/MESOS-3697 Repository: mesos Description --- This commit will introduce a `test` target to the CMake build system. The semantics of this target are very similar to the autotools build system, in that they build the tests, but do not run them. Accomplishing this is somewhat complex in CMake, because CMake expects to control the `test` target itself. We work around this by (1) silencing the warning that `test` is a reserved target, (2) removing the call to `enable_testing` that sets up CTest, and (3) adding our own `test` target. As an additional insurance policy, we error out if any of the `BUILD_TESTING` flags are defined, which would indicate that `include(CTest)` was called. Diffs - CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc src/tests/CMakeLists.txt b7adf4f3126611a57baf4f62b62b208bc0236da5 Diff: https://reviews.apache.org/r/55637/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55635: Stout: Removed usage of CTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55635/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3697 https://issues.apache.org/jira/browse/MESOS-3697 Repository: mesos Description --- CTest automatically defines a `test` target, which directly impedes the resolution of MESOS-3697. Since Stout doesn't make use of CTest anyway, this commit will simply retire it from Stout. Diffs - 3rdparty/stout/tests/CMakeLists.txt a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae Diff: https://reviews.apache.org/r/55635/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55632: CMake: Disabled rpath to silence CMake warning on OS X.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55632/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- CMake: Disabled rpath to silence CMake warning on OS X. Diffs - cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc Diff: https://reviews.apache.org/r/55632/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55607: CMake: Added configuration of test scripts in the bin/ directory.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55607/ --- (Updated Jan. 17, 2017, 8:34 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Fix typos in comments. Bugs: MESOS-6757 https://issues.apache.org/jira/browse/MESOS-6757 Repository: mesos Description --- This resolves MESOS-6757. Diffs (updated) - cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc Diff: https://reviews.apache.org/r/55607/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55607: CMake: Added configuration of test scripts in the bin/ directory.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55607/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-6757 https://issues.apache.org/jira/browse/MESOS-6757 Repository: mesos Description --- This resolves MESOS-6757. Diffs - cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc Diff: https://reviews.apache.org/r/55607/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55604: CMake: Transitioned Libprocess to automatic source grouping.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55604/ --- (Updated Jan. 17, 2017, 6:42 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Fixup file matching pattern. Bugs: MESOS-3542 https://issues.apache.org/jira/browse/MESOS-3542 Repository: mesos Description --- Libprocess currently manually groups its source code by folder in order to present the source nicely in IDEs like XCode and Visual Studio. With the recently-introduced CMake function, `GROUP_SOURCE`, this process is automated away. This commit will remove these manual groups and replace it with a call to this function. Diffs (updated) - 3rdparty/libprocess/cmake/ProcessConfigure.cmake 873f41d844faa0d442f77411e94314a89be5f046 3rdparty/libprocess/src/CMakeLists.txt 60f0e76dfd237d9a12a366b413802d1a96892b55 3rdparty/libprocess/src/tests/CMakeLists.txt 0b2660cb16f5d8d8dc66e6995061d0b832182351 Diff: https://reviews.apache.org/r/55604/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55601: CMake: Added source groups for agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55601/ --- (Updated Jan. 17, 2017, 6:41 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Fixup file matching pattern. Bugs: MESOS-3542 https://issues.apache.org/jira/browse/MESOS-3542 Repository: mesos Description --- This commit will continue our effort to break libmesos into a modular set of libraries that can be built and linked independently, and away from the approach of creating a single, monolithic libmesos library. This effort is tracked in MESOS-3542. This commit will specifically address the problem of breaking agent code into a self-contained source group, which can then be exported as its own library. This code also has the benefit of making IDEs display the agent code in a hierarchical directory structure. Diffs (updated) - cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc src/CMakeLists.txt ce71afc73f85a70cd8f97a6e913662ff7ef0d94c src/slave/cmake/AgentConfigure.cmake 1582127ccce773af6031a5b09252192b05a13cdc src/slave/cmake/AgentSourceList.cmake PRE-CREATION src/slave/container_loggers/CMakeLists.txt b46360fb1ced188102c285c914cc0d146c6db5e1 src/slave/qos_controllers/CMakeLists.txt 65ab338a71276a77e43af962fbc9b76e050efca6 Diff: https://reviews.apache.org/r/55601/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55602: CMake: Added source groups for master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55602/ --- (Updated Jan. 17, 2017, 6:41 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Fixup file matching pattern. Bugs: MESOS-3542 https://issues.apache.org/jira/browse/MESOS-3542 Repository: mesos Description --- This commit will continue our effort to break libmesos into a modular set of libraries that can be built and linked independently, and away from the approach of creating a single, monolithic libmesos library. This effort is tracked in MESOS-3542. This commit will specifically address the problem of breaking master code into a self-contained source group, which can then be exported as its own library. This code also has the benefit of making IDEs display the master code in a hierarchical directory structure. Diffs (updated) - src/CMakeLists.txt ce71afc73f85a70cd8f97a6e913662ff7ef0d94c src/master/cmake/MasterSourceList.cmake PRE-CREATION Diff: https://reviews.apache.org/r/55602/diff/ Testing --- Thanks, Alex Clemmer
Re: Review Request 55600: CMake: Transitioned Stout to automatic source grouping.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55600/ --- (Updated Jan. 17, 2017, 6:41 a.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes --- Fixup file matching pattern. Repository: mesos Description --- Stout currently manually groups its source code by folder in order to present the source nicely in IDEs like XCode and Visual Studio. With the recently-introduced CMake function, `GROUP_SOURCE`, this process is automated away. This commit will remove these manual groups and replace it with a call to this function. Diffs (updated) - 3rdparty/stout/cmake/StoutConfigure.cmake bc27ac687bae4e1798eece562027ba33c6b32348 3rdparty/stout/tests/CMakeLists.txt a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae Diff: https://reviews.apache.org/r/55600/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55604: CMake: Transitioned Libprocess to automatic source grouping.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55604/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3542 https://issues.apache.org/jira/browse/MESOS-3542 Repository: mesos Description --- Libprocess currently manually groups its source code by folder in order to present the source nicely in IDEs like XCode and Visual Studio. With the recently-introduced CMake function, `GROUP_SOURCE`, this process is automated away. This commit will remove these manual groups and replace it with a call to this function. Diffs - 3rdparty/libprocess/cmake/ProcessConfigure.cmake 873f41d844faa0d442f77411e94314a89be5f046 3rdparty/libprocess/src/CMakeLists.txt 60f0e76dfd237d9a12a366b413802d1a96892b55 3rdparty/libprocess/src/tests/CMakeLists.txt 0b2660cb16f5d8d8dc66e6995061d0b832182351 Diff: https://reviews.apache.org/r/55604/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55602: CMake: Added source groups for master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55602/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3542 https://issues.apache.org/jira/browse/MESOS-3542 Repository: mesos Description --- This commit will continue our effort to break libmesos into a modular set of libraries that can be built and linked independently, and away from the approach of creating a single, monolithic libmesos library. This effort is tracked in MESOS-3542. This commit will specifically address the problem of breaking master code into a self-contained source group, which can then be exported as its own library. This code also has the benefit of making IDEs display the master code in a hierarchical directory structure. Diffs - src/CMakeLists.txt ce71afc73f85a70cd8f97a6e913662ff7ef0d94c src/master/cmake/MasterSourceList.cmake PRE-CREATION Diff: https://reviews.apache.org/r/55602/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55601: CMake: Added source groups for agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55601/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Bugs: MESOS-3542 https://issues.apache.org/jira/browse/MESOS-3542 Repository: mesos Description --- This commit will continue our effort to break libmesos into a modular set of libraries that can be built and linked independently, and away from the approach of creating a single, monolithic libmesos library. This effort is tracked in MESOS-3542. This commit will specifically address the problem of breaking agent code into a self-contained source group, which can then be exported as its own library. This code also has the benefit of making IDEs display the agent code in a hierarchical directory structure. Diffs - cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc src/CMakeLists.txt ce71afc73f85a70cd8f97a6e913662ff7ef0d94c src/slave/cmake/AgentConfigure.cmake 1582127ccce773af6031a5b09252192b05a13cdc src/slave/cmake/AgentSourceList.cmake PRE-CREATION src/slave/container_loggers/CMakeLists.txt b46360fb1ced188102c285c914cc0d146c6db5e1 src/slave/qos_controllers/CMakeLists.txt 65ab338a71276a77e43af962fbc9b76e050efca6 Diff: https://reviews.apache.org/r/55601/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55600: CMake: Transitioned Stout to automatic source grouping.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55600/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- Stout currently manually groups its source code by folder in order to present the source nicely in IDEs like XCode and Visual Studio. With the recently-introduced CMake function, `GROUP_SOURCE`, this process is automated away. This commit will remove these manual groups and replace it with a call to this function. Diffs - 3rdparty/stout/cmake/StoutConfigure.cmake bc27ac687bae4e1798eece562027ba33c6b32348 3rdparty/stout/tests/CMakeLists.txt a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae Diff: https://reviews.apache.org/r/55600/diff/ Testing --- Thanks, Alex Clemmer
Review Request 55599: CMake: Added `GroupSource` function to automate IDE source grouping.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55599/ --- Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Repository: mesos Description --- CMake allows users to declare groups of source files, which it uses to (among other things) present source in a directory-like tree of files in IDEs like XCode and Visual Studio. Currently this is a manual process: we group the source in each folder manually and declare it as a source group to CMake. This commit will introduce a CMake macro that automates this process away, providing control over many aspects, such as where the group tree should be rooted, what the root should be named, and so on. This macro indirectly partially addresses MESOS-3542, which will help us to separate out Mesos builds into many libraries, rather than one single, monolithic libmesos. Diffs - 3rdparty/stout/cmake/GroupSource.cmake PRE-CREATION Diff: https://reviews.apache.org/r/55599/diff/ Testing --- Thanks, Alex Clemmer
