You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@mesosphere.io> on 2017/10/04 18:53:32 UTC
Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/
-----------------------------------------------------------
Review request for mesos and Benjamin Bannier.
Bugs: MESOS-8052
https://issues.apache.org/jira/browse/MESOS-8052
Repository: mesos
Description
-------
The protobuf hpp and cpp files for `protobuf_tests.proto` should be
built after `BUNDLED_DEPS`, not within it.
Diffs
-----
3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
Diff: https://reviews.apache.org/r/62777/diff/1/
Testing
-------
make -j4 check
Thanks,
Chun-Hung Hsiao
Re: Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/#review188045
-----------------------------------------------------------
PASS: Mesos patch 62777 was successfully built and tested.
Reviews applied: `['62777']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62777
- Mesos Reviewbot Windows
On Oct. 13, 2017, 6:24 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62777/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2017, 6:24 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-8052
> https://issues.apache.org/jira/browse/MESOS-8052
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The protobuf hpp and cpp files for `protobuf_tests.proto` should be
> built after `BUNDLED_DEPS`, not within it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
>
>
> Diff: https://reviews.apache.org/r/62777/diff/2/
>
>
> Testing
> -------
>
> make -j4 check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/#review191140
-----------------------------------------------------------
PASS: Mesos patch 62777 was successfully built and tested.
Reviews applied: `['62777']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62777
- Mesos Reviewbot Windows
On Nov. 15, 2017, 9:40 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62777/
> -----------------------------------------------------------
>
> (Updated Nov. 15, 2017, 9:40 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-8052
> https://issues.apache.org/jira/browse/MESOS-8052
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The protobuf hpp and cpp files for `protobuf_tests.proto` should be
> built after `BUNDLED_DEPS`, not within it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
>
>
> Diff: https://reviews.apache.org/r/62777/diff/3/
>
>
> Testing
> -------
>
> make -j4 check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/
-----------------------------------------------------------
(Updated Nov. 15, 2017, 9:40 p.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Addressed bbannier's comment.
Bugs: MESOS-8052
https://issues.apache.org/jira/browse/MESOS-8052
Repository: mesos
Description
-------
The protobuf hpp and cpp files for `protobuf_tests.proto` should be
built after `BUNDLED_DEPS`, not within it.
Diffs (updated)
-----
3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
Diff: https://reviews.apache.org/r/62777/diff/3/
Changes: https://reviews.apache.org/r/62777/diff/2-3/
Testing
-------
make -j4 check
Thanks,
Chun-Hung Hsiao
Re: Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/#review191107
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/stout/Makefile.am
Line 122 (original), 122 (patched)
<https://reviews.apache.org/r/62777/#comment268696>
The dependency on protobuf only works if we used the bundled protobuf; for an unbundled protobuf the build stamp file would never get created.
Could we instead depend on `BUNDLED_DEPS`? While this might build more than we want, I believe it should be populated correctly regardless of whether we use bundled deps or not.
- Benjamin Bannier
On Oct. 13, 2017, 8:24 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62777/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2017, 8:24 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-8052
> https://issues.apache.org/jira/browse/MESOS-8052
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The protobuf hpp and cpp files for `protobuf_tests.proto` should be
> built after `BUNDLED_DEPS`, not within it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
>
>
> Diff: https://reviews.apache.org/r/62777/diff/2/
>
>
> Testing
> -------
>
> make -j4 check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/
-----------------------------------------------------------
(Updated Oct. 13, 2017, 6:24 p.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Addressed bbannier's comments.
Bugs: MESOS-8052
https://issues.apache.org/jira/browse/MESOS-8052
Repository: mesos
Description
-------
The protobuf hpp and cpp files for `protobuf_tests.proto` should be
built after `BUNDLED_DEPS`, not within it.
Diffs (updated)
-----
3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
Diff: https://reviews.apache.org/r/62777/diff/2/
Changes: https://reviews.apache.org/r/62777/diff/1-2/
Testing
-------
make -j4 check
Thanks,
Chun-Hung Hsiao
Re: Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/#review187127
-----------------------------------------------------------
PASS: Mesos patch 62777 was successfully built and tested.
Reviews applied: `['62777']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62777
- Mesos Reviewbot Windows
On Oct. 4, 2017, 2:53 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62777/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2017, 2:53 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-8052
> https://issues.apache.org/jira/browse/MESOS-8052
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The protobuf hpp and cpp files for `protobuf_tests.proto` should be
> built after `BUNDLED_DEPS`, not within it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
>
>
> Diff: https://reviews.apache.org/r/62777/diff/1/
>
>
> Testing
> -------
>
> make -j4 check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/#review187177
-----------------------------------------------------------
3rdparty/stout/Makefile.am
Line 218 (original), 219 (patched)
<https://reviews.apache.org/r/62777/#comment264087>
Hmm, adding these files to the normal sources incorrectly models that some other translation units depend on the generated headers (namely `tests/protobuf_tests.cpp`). This can leads builds starting the compile of `protobuf_tests.cpp` before `protobuf_tests.pb.h` is generated (can happen in both parallel and sequential builds). This is one aspect that adding the generated files to `BUILT_SOURCES` prevents.
How about we explicitly depend on `BUNDLED_DEPS` in the protobuf generation rule, and still add the generated proto files to `BUILT_SOURCES`?
%.pb.cc %.pb.h: tests/%.proto $(BUNDLED_DEPS)
...
- Benjamin Bannier
On Oct. 4, 2017, 8:53 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62777/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2017, 8:53 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-8052
> https://issues.apache.org/jira/browse/MESOS-8052
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The protobuf hpp and cpp files for `protobuf_tests.proto` should be
> built after `BUNDLED_DEPS`, not within it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
>
>
> Diff: https://reviews.apache.org/r/62777/diff/1/
>
>
> Testing
> -------
>
> make -j4 check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/#review187138
-----------------------------------------------------------
PASS: Mesos patch 62777 was successfully built and tested.
Reviews applied: `['62777']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62777
- Mesos Reviewbot Windows
On Oct. 4, 2017, 6:53 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62777/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2017, 6:53 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-8052
> https://issues.apache.org/jira/browse/MESOS-8052
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The protobuf hpp and cpp files for `protobuf_tests.proto` should be
> built after `BUNDLED_DEPS`, not within it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
>
>
> Diff: https://reviews.apache.org/r/62777/diff/1/
>
>
> Testing
> -------
>
> make -j4 check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62777: Fixed build dependency for
`protobuf_tests.proto`.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62777/#review187173
-----------------------------------------------------------
3rdparty/stout/Makefile.am
Line 122 (original), 122 (patched)
<https://reviews.apache.org/r/62777/#comment264086>
We generally assume GNU make so we can produce multiple targets from a single rule invocation, but for some reason this failed here, https://reviews.apache.org/r/62777/#last-review.
The GNU extension we rely on makes sure to invoke multi-target rules exactly once; otherwise rules producing multiple output files could be invoked `num_target` times in parallel where the different processes race on the same output files, possibly producing junk.
I tested this with a pattern rule
%.pb.cc %pb.h: tests/%.proto
$(PROTOC) $(PROTOCFLAGS) --cpp_out=$(builddir) $^
with looped `make` invocations like
$ rm protobuf_tests.pb.cc protobuf_tests.pb.h && make -j stout_tests-protobuf_tests.pb.o
The form in this patch seems to produce multiple `protoc` invocations pretty often while the one suggested above worked fine in the limited number of tests I ran.
- Benjamin Bannier
On Oct. 4, 2017, 8:53 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62777/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2017, 8:53 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-8052
> https://issues.apache.org/jira/browse/MESOS-8052
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The protobuf hpp and cpp files for `protobuf_tests.proto` should be
> built after `BUNDLED_DEPS`, not within it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5
>
>
> Diff: https://reviews.apache.org/r/62777/diff/1/
>
>
> Testing
> -------
>
> make -j4 check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>