> On March 28, 2019, 10:06 a.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 489-492 (patched)
> > <https://reviews.apache.org/r/70302/diff/2/?file=2134667#file2134667line495>
> >
> > 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
> 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
>
>
