Review Request 70302: Adjusted CSI v0 proto compilation.

2019-03-25 Thread Chun-Hung Hsiao 

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

Review request for mesos, Benjamin Bannier and Joseph Wu.


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


Repository: mesos


Description
---

This patch makes the following adjustments so we can build CSI v0 and v1
in the future:

* Standardize placements for third-party proto files: `csi.proto` from
  CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
  `build/include/csi/v1` in the future. Dependent proto files should
  import `csi/v0/csi.proto` or `csi/v1/csi.proto`.

* Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
  future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
  Since we don't insteall CSI headers, for the installed `v0.hpp` and
  `v1.hpp` headers to work, users must ensure that the CSI headers can
  be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.


Diffs
-

  3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
  3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
  3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
  3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
  include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
  src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
  src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
  src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
  src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
  src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
  src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
  src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
  src/resource_provider/storage/disk_profile.proto 
1c97e9c62fb3154d6048d0e55352a1276adcbca8 
  src/resource_provider/storage/disk_profile_utils.hpp 
8a83a15ba555ce66bbb86b8df72178bce17a615a 
  src/tests/csi_utils_tests.cpp PRE-CREATION 
  src/tests/disk_profile_adaptor_tests.cpp 
0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
  src/tests/mock_csi_plugin.hpp 6897fbc878f1e2f5b9e9c402b09358c187af79a0 


Diff: https://reviews.apache.org/r/70302/diff/1/


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

2019-03-26 Thread Mesos Reviewbot Windows 

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['70245', '70168', '70213', '70214', '70216', '70215', 
'70217', '70284', '70222', '70285', '70223', '70225', '70247', '70258', 
'70248', '70302']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3016/mesos-review-70302

Relevant logs:

- 
[mesos-tests-cmake.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3016/mesos-review-70302/logs/mesos-tests-cmake.log):

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\recordio.c(170):
 warning C4267: '=': conversion from 'size_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\d

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

2019-03-26 Thread Benjamin Bannier 

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


Fix it, then Ship it!




Looks good module two issues.


3rdparty/CMakeLists.txt
Line 1 (original), 1 (patched)


I would suggest we move this patch in between 
https://reviews.apache.org/r/70248/ and https://reviews.apache.org/r/70247/.



include/csi/spec.hpp
Line 1 (original), 1 (patched)


Let's explicitly include this file in `src/csi/volume_manager.hpp`.


- Benjamin Bannier


On March 26, 2019, 6:13 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70302/
> ---
> 
> (Updated March 26, 2019, 6:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-9624
> https://issues.apache.org/jira/browse/MESOS-9624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the following adjustments so we can build CSI v0 and v1
> in the future:
> 
> * Standardize placements for third-party proto files: `csi.proto` from
>   CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
>   `build/include/csi/v1` in the future. Dependent proto files should
>   import `csi/v0/csi.proto` or `csi/v1/csi.proto`.
> 
> * Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
>   future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
>   Since we don't insteall CSI headers, for the installed `v0.hpp` and
>   `v1.hpp` headers to work, users must ensure that the CSI headers can
>   be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
>   3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
>   3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
>   3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
>   include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
>   src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
>   src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
>   src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
>   src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
>   src/resource_provider/storage/disk_profile.proto 
> 1c97e9c62fb3154d6048d0e55352a1276adcbca8 
>   src/resource_provider/storage/disk_profile_utils.hpp 
> 8a83a15ba555ce66bbb86b8df72178bce17a615a 
>   src/tests/csi_utils_tests.cpp PRE-CREATION 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
>   src/tests/mock_csi_plugin.hpp 6897fbc878f1e2f5b9e9c402b09358c187af79a0 
> 
> 
> Diff: https://reviews.apache.org/r/70302/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

2019-03-26 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: [70302, 70248, 70258, 70247, 70225, 70223, 70285, 70222, 
70284, 70217, 70215, 70216, 70214, 70213, 70168, 70245, 70316, 70315, 70314, 
70313]

Error:
2019-03-27 04:43:50 URL:https://reviews.apache.org/r/70168/diff/raw/ 
[65403/65403] -> "70168.patch" [1]
error: patch failed: src/resource_provider/storage/provider.cpp:2704
error: src/resource_provider/storage/provider.cpp: patch does not apply

- Mesos Reviewbot


On March 26, 2019, 5:13 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70302/
> ---
> 
> (Updated March 26, 2019, 5:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-9624
> https://issues.apache.org/jira/browse/MESOS-9624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the following adjustments so we can build CSI v0 and v1
> in the future:
> 
> * Standardize placements for third-party proto files: `csi.proto` from
>   CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
>   `build/include/csi/v1` in the future. Dependent proto files should
>   import `csi/v0/csi.proto` or `csi/v1/csi.proto`.
> 
> * Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
>   future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
>   Since we don't insteall CSI headers, for the installed `v0.hpp` and
>   `v1.hpp` headers to work, users must ensure that the CSI headers can
>   be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
>   3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
>   3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
>   3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
>   include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
>   src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
>   src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
>   src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
>   src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
>   src/resource_provider/storage/disk_profile.proto 
> 1c97e9c62fb3154d6048d0e55352a1276adcbca8 
>   src/resource_provider/storage/disk_profile_utils.hpp 
> 8a83a15ba555ce66bbb86b8df72178bce17a615a 
>   src/tests/csi_utils_tests.cpp PRE-CREATION 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
>   src/tests/mock_csi_plugin.hpp 6897fbc878f1e2f5b9e9c402b09358c187af79a0 
> 
> 
> Diff: https://reviews.apache.org/r/70302/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

2019-03-26 Thread Mesos Reviewbot Windows 

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



FAIL: Failed to apply the dependent review: 70216.

Failed command: `python.exe .\support\apply-reviews.py -n -r 70216`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3023/mesos-review-70302

Relevant logs:

- 
[apply-review-70216.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3023/mesos-review-70302/logs/apply-review-70216.log):

```
error: patch failed: src/resource_provider/storage/provider.cpp:2130
error: src/resource_provider/storage/provider.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On March 25, 2019, 10:13 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70302/
> ---
> 
> (Updated March 25, 2019, 10:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-9624
> https://issues.apache.org/jira/browse/MESOS-9624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the following adjustments so we can build CSI v0 and v1
> in the future:
> 
> * Standardize placements for third-party proto files: `csi.proto` from
>   CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
>   `build/include/csi/v1` in the future. Dependent proto files should
>   import `csi/v0/csi.proto` or `csi/v1/csi.proto`.
> 
> * Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
>   future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
>   Since we don't insteall CSI headers, for the installed `v0.hpp` and
>   `v1.hpp` headers to work, users must ensure that the CSI headers can
>   be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
>   3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
>   3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
>   3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
>   include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
>   src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
>   src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
>   src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
>   src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
>   src/resource_provider/storage/disk_profile.proto 
> 1c97e9c62fb3154d6048d0e55352a1276adcbca8 
>   src/resource_provider/storage/disk_profile_utils.hpp 
> 8a83a15ba555ce66bbb86b8df72178bce17a615a 
>   src/tests/csi_utils_tests.cpp PRE-CREATION 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
>   src/tests/mock_csi_plugin.hpp 6897fbc878f1e2f5b9e9c402b09358c187af79a0 
> 
> 
> Diff: https://reviews.apache.org/r/70302/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

2019-03-26 Thread Chun-Hung Hsiao 


> On March 26, 2019, 3:44 p.m., Benjamin Bannier wrote:
> > 3rdparty/CMakeLists.txt
> > Line 1 (original), 1 (patched)
> > 
> >
> > I would suggest we move this patch in between 
> > https://reviews.apache.org/r/70248/ and https://reviews.apache.org/r/70247/.

This cannot be moved before r/72048 because it relies on that some `csi.proto` 
dependencies have been removed from r/70248. Dropping.


> On March 26, 2019, 3:44 p.m., Benjamin Bannier wrote:
> > include/csi/spec.hpp
> > Line 1 (original), 1 (patched)
> > 
> >
> > Let's explicitly include this file in `src/csi/volume_manager.hpp`.

Reordered the patches so no need to do the header inclusion. Let's keep 
`volume_manager.hpp` CSI-version agnostic.


- Chun-Hung


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


On March 26, 2019, 5:13 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70302/
> ---
> 
> (Updated March 26, 2019, 5:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-9624
> https://issues.apache.org/jira/browse/MESOS-9624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the following adjustments so we can build CSI v0 and v1
> in the future:
> 
> * Standardize placements for third-party proto files: `csi.proto` from
>   CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
>   `build/include/csi/v1` in the future. Dependent proto files should
>   import `csi/v0/csi.proto` or `csi/v1/csi.proto`.
> 
> * Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
>   future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
>   Since we don't insteall CSI headers, for the installed `v0.hpp` and
>   `v1.hpp` headers to work, users must ensure that the CSI headers can
>   be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
>   3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
>   3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
>   3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
>   include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
>   src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
>   src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
>   src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
>   src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
>   src/resource_provider/storage/disk_profile.proto 
> 1c97e9c62fb3154d6048d0e55352a1276adcbca8 
>   src/resource_provider/storage/disk_profile_utils.hpp 
> 8a83a15ba555ce66bbb86b8df72178bce17a615a 
>   src/tests/csi_utils_tests.cpp PRE-CREATION 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
>   src/tests/mock_csi_plugin.hpp 6897fbc878f1e2f5b9e9c402b09358c187af79a0 
> 
> 
> Diff: https://reviews.apache.org/r/70302/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

2019-03-27 Thread Chun-Hung Hsiao 

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

(Updated March 27, 2019, 3:27 p.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This patch makes the following adjustments so we can build CSI v0 and v1
in the future:

* Standardize placements for third-party proto files: `csi.proto` from
  CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
  `build/include/csi/v1` in the future. Dependent proto files should
  import `csi/v0/csi.proto` or `csi/v1/csi.proto`.

* Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
  future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
  Since we don't insteall CSI headers, for the installed `v0.hpp` and
  `v1.hpp` headers to work, users must ensure that the CSI headers can
  be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
  3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
  3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
  3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
  include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
  src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
  src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
  src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
  src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
  src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
  src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
  src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
  src/resource_provider/storage/disk_profile.proto 
1c97e9c62fb3154d6048d0e55352a1276adcbca8 
  src/resource_provider/storage/disk_profile_utils.hpp 
8a83a15ba555ce66bbb86b8df72178bce17a615a 
  src/tests/csi_utils_tests.cpp PRE-CREATION 
  src/tests/disk_profile_adaptor_tests.cpp 
0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
  src/tests/mock_csi_plugin.hpp 6897fbc878f1e2f5b9e9c402b09358c187af79a0 


Diff: https://reviews.apache.org/r/70302/diff/2/

Changes: https://reviews.apache.org/r/70302/diff/1-2/


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

2019-03-28 Thread Benjamin Bannier 

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




src/CMakeLists.txt
Line 123 (original), 118 (patched)


If we use this here let's also init the variable above; else remove the 
usage here.



src/CMakeLists.txt
Line 126 (original), 121 (patched)


Ditto.



src/Makefile.am
Lines 489-492 (patched)


We don't typically directly copy 3rdparty sources into our build tree; 
let's not start doing it here.

Let's instead build our artifacts by directly referencing the unpacked 
artifacts. We can adjust the macro to do just that. Above approach also fails 
in `make distcheck` as it places files in the build tree which are not removed 
by any `clean` rule.


- Benjamin Bannier


On March 27, 2019, 4:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70302/
> ---
> 
> (Updated March 27, 2019, 4:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-9624
> https://issues.apache.org/jira/browse/MESOS-9624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the following adjustments so we can build CSI v0 and v1
> in the future:
> 
> * Standardize placements for third-party proto files: `csi.proto` from
>   CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
>   `build/include/csi/v1` in the future. Dependent proto files should
>   import `csi/v0/csi.proto` or `csi/v1/csi.proto`.
> 
> * Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
>   future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
>   Since we don't insteall CSI headers, for the installed `v0.hpp` and
>   `v1.hpp` headers to work, users must ensure that the CSI headers can
>   be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
>   3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
>   3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
>   3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
>   include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
>   src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
>   src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
>   src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
>   src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
>   src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
>   src/resource_provider/storage/disk_profile.proto 
> 1c97e9c62fb3154d6048d0e55352a1276adcbca8 
>   src/resource_provider/storage/disk_profile_utils.hpp 
> 8a83a15ba555ce66bbb86b8df72178bce17a615a 
>   src/tests/csi_utils_tests.cpp PRE-CREATION 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
>   src/tests/mock_csi_plugin.hpp 6897fbc878f1e2f5b9e9c402b09358c187af79a0 
> 
> 
> Diff: https://reviews.apache.org/r/70302/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

2019-03-28 Thread Chun-Hung Hsiao 


> On March 28, 2019, 10:06 a.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 489-492 (patched)
> > 
> >
> > We don't typically directly copy 3rdparty sources into our build tree; 
> > let's not start doing it here.
> > 
> > Let's instead build our artifacts by directly referencing the unpacked 
> > artifacts. We can adjust the macro to do just that. Above approach also 
> > fails in `make distcheck` as it places files in the build tree which are 
> > not removed by any `clean` rule.

It depends if we want to allow our own proto to refer to these protos in the 
future. If we want to allow that, the problem is that it won't work with protoc.

Consider that we have `src/csi/v0_state.proto` and `src/csi/v1_state.proto`, 
both import `csi.proto` of respective version, and the directory layout is as 
follows:
```
build/3rdparty/csi_v0-0.2.0/csi.proto
build/3rdparty/csi_v1-1.1.0/csi.proto
```
Apprantly we cannot do `protoc -Ibuild/3rdparty/csi_v0-0.2.0/ 
-Ibuild/3rdparty/csi_v1-1.1.0/`. Instead, we might have to write specific rules 
to compile these two protos, like:
```
csi/v0_%.pb.h csi/v0_%.pb.cc: csi/v0_%.proto
$(PROTOC) $(PROTOCFLAGS) -I$(CSI_V0) --cpp_out=. $^

csi/v1_%.pb.h csi/v1_%.pb.cc: csi/v1_%.proto
$(PROTOC) $(PROTOCFLAGS) -I$(CSI_V1) --cpp_out=. $^
```
We'll need to do a similar workaround in our cmake rules.

Now, workaround protoc is actually a small problem. The bigger problem is that, 
by doing the above, the generated code would both include `csi.pb.h`.
Again, we cannot do `g++ -Ibuild/include/csi/v0 -Ibuild/include/csi/v1`, so we 
have to also write specific rules for compiling the generated code.

The alternative I proposed here, is that we first place `csi.proto`s with the 
following directory layout:
```
build/include/csi/v0/csi.proto
build/include/csi/v1/csi.proto
```
Then, in `src/csi/v0_state.proto` we do `import "csi/v0/csi.proto";`, and in 
`src/csi/v1_state.proto` we do `import "csi/v1/csi.proto`.
As a result, `csi/v0/csi.pb.h` will be included in the generated files of the 
former, and `csi/v1/csi.pb.h` will be included in that of the latter.

That said, since we now have introduced unversioned protos, we currently don't 
have any proto depending on the CSI proto of a specific version. But, we'll 
need a resolution unless we are sure that there will never be such a need.

WDYT?


- Chun-Hung


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


On March 27, 2019, 3:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70302/
> ---
> 
> (Updated March 27, 2019, 3:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-9624
> https://issues.apache.org/jira/browse/MESOS-9624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the following adjustments so we can build CSI v0 and v1
> in the future:
> 
> * Standardize placements for third-party proto files: `csi.proto` from
>   CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
>   `build/include/csi/v1` in the future. Dependent proto files should
>   import `csi/v0/csi.proto` or `csi/v1/csi.proto`.
> 
> * Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
>   future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
>   Since we don't insteall CSI headers, for the installed `v0.hpp` and
>   `v1.hpp` headers to work, users must ensure that the CSI headers can
>   be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
>   3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
>   3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
>   3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
>   include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
>   src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
>   src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
>   src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
>   src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
>   src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
>   src/resource_provider/storage/disk_profile.proto 
> 1c97e9c62fb3154d6048d0e55352a1276adcbca8 
>   src/resource_provider/storage/disk_profile_utils.hpp 
> 8a83a15ba555ce66bbb86b8df72178bce17