You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Srinivas Brahmaroutu <sr...@us.ibm.com> on 2016/12/08 18:19:30 UTC
Re: Review Request 50415: Added build step to build Java Protobuf.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50415/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 6:19 p.m.)
Review request for mesos, Alex Clemmer and Joseph Wu.
Bugs: MESOS-5902
https://issues.apache.org/jira/browse/MESOS-5902
Repository: mesos
Description (updated)
-------
Added config, build and install steps for the External project
protobuf_java.
'make probuf_java' produces the
${PROTOBUF_JAVA_ROOT}/java/target/protobuf-java-2.6.1.jar
Diffs (updated)
-----
3rdparty/CMakeLists.txt a340b29fc1cc8b68bf74bfc0c9c2274f0af84eee
Diff: https://reviews.apache.org/r/50415/diff/
Testing
-------
cmake && make protobuf-2.6.1-java
check if 3rdparty/protobuf-2.6.1/src/protobuf-2.6.1-java/java/target/protobuf-java-2.6.1.jar
Thanks,
Srinivas Brahmaroutu
Re: Review Request 50415: Added build step to build Java Protobuf.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50415/#review167521
-----------------------------------------------------------
Closing this review due to inactivity. Please see our [guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md) for reopening reviews.
- Joris Van Remoortere
On Dec. 10, 2016, 7:18 p.m., Srinivas Brahmaroutu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50415/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 7:18 p.m.)
>
>
> Review request for mesos, Alex Clemmer and Joseph Wu.
>
>
> Bugs: MESOS-5902
> https://issues.apache.org/jira/browse/MESOS-5902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added config, build and install steps for the External project
> protobuf_java.
> 'make probuf_java' produces the
> ${PROTOBUF_JAVA_ROOT}/java/target/protobuf-java-2.6.1.jar
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt a340b29fc1cc8b68bf74bfc0c9c2274f0af84eee
>
>
> Diff: https://reviews.apache.org/r/50415/diff/8/
>
>
> Testing
> -------
>
> cmake && make protobuf-2.6.1-java
> check if 3rdparty/protobuf-2.6.1/src/protobuf-2.6.1-java/java/target/protobuf-java-2.6.1.jar
>
>
> Thanks,
>
> Srinivas Brahmaroutu
>
>
Re: Review Request 50415: Added build step to build Java Protobuf.
Posted by Srinivas Brahmaroutu <sr...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50415/
-----------------------------------------------------------
(Updated Dec. 10, 2016, 7:18 p.m.)
Review request for mesos, Alex Clemmer and Joseph Wu.
Bugs: MESOS-5902
https://issues.apache.org/jira/browse/MESOS-5902
Repository: mesos
Description
-------
Added config, build and install steps for the External project
protobuf_java.
'make probuf_java' produces the
${PROTOBUF_JAVA_ROOT}/java/target/protobuf-java-2.6.1.jar
Diffs (updated)
-----
3rdparty/CMakeLists.txt a340b29fc1cc8b68bf74bfc0c9c2274f0af84eee
Diff: https://reviews.apache.org/r/50415/diff/
Testing
-------
cmake && make protobuf-2.6.1-java
check if 3rdparty/protobuf-2.6.1/src/protobuf-2.6.1-java/java/target/protobuf-java-2.6.1.jar
Thanks,
Srinivas Brahmaroutu
Re: Review Request 50415: Added build step to build Java Protobuf.
Posted by Srinivas Brahmaroutu <sr...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50415/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 11:04 p.m.)
Review request for mesos, Alex Clemmer and Joseph Wu.
Bugs: MESOS-5902
https://issues.apache.org/jira/browse/MESOS-5902
Repository: mesos
Description
-------
Added config, build and install steps for the External project
protobuf_java.
'make probuf_java' produces the
${PROTOBUF_JAVA_ROOT}/java/target/protobuf-java-2.6.1.jar
Diffs (updated)
-----
3rdparty/CMakeLists.txt a340b29fc1cc8b68bf74bfc0c9c2274f0af84eee
Diff: https://reviews.apache.org/r/50415/diff/
Testing
-------
cmake && make protobuf-2.6.1-java
check if 3rdparty/protobuf-2.6.1/src/protobuf-2.6.1-java/java/target/protobuf-java-2.6.1.jar
Thanks,
Srinivas Brahmaroutu
Re: Review Request 50415: Added build step to build Java Protobuf.
Posted by Srinivas Brahmaroutu <sr...@us.ibm.com>.
> On Dec. 8, 2016, 8:59 p.m., Alex Clemmer wrote:
> > 3rdparty/CMakeLists.txt, line 347
> > <https://reviews.apache.org/r/50415/diff/6/?file=1579858#file1579858line347>
> >
> > If I'm understanding this correctly, this will download a fresh copy of the protobuf tarball, and then build only the Java parts of it.
> >
> > Since we're already downloading this for the normal Protobuf build, it seems like it would be easier to get rid of the `ExternalProject_Add` call and simply append the java config, build, and install commands to the protobuf config, build, and install commands. For example:
> >
> > ```
> > set(PROTOBUF_BUILD_CMD ${PROTOBUF_BUILD_CMD} ${PROTOBUF_JAVA_BUILD_CMD})
> > ```
> >
> > This should cause the normal protobuf build to build the Java stuff as well, no?
>
> Srinivas Brahmaroutu wrote:
> I prefer to build the jar as a step for Java and I will add another step for Python. Hope this works.
>
> Alex Clemmer wrote:
> Can you speak a little more about why you prefer it? I personally would prefer to keep the Java, native, and Python builds of PB all coming from the same source tree just because it's less error-prone.
I fixed the that issue as you instructed, now the same source tree is used by Java and Python builds. Adding a Java step and then a Python step to the same project will just execute the step after building the protoc.
Source code is fetched, C++ build steps are performed.
Java step is invoked (depends on install step above) and Jar is built from the same source. java/mvn install
Python step is invoked (depends on install step above) and egg is built from the same source. python setup.py build
- Srinivas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50415/#review158565
-----------------------------------------------------------
On Dec. 10, 2016, 7:18 p.m., Srinivas Brahmaroutu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50415/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 7:18 p.m.)
>
>
> Review request for mesos, Alex Clemmer and Joseph Wu.
>
>
> Bugs: MESOS-5902
> https://issues.apache.org/jira/browse/MESOS-5902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added config, build and install steps for the External project
> protobuf_java.
> 'make probuf_java' produces the
> ${PROTOBUF_JAVA_ROOT}/java/target/protobuf-java-2.6.1.jar
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt a340b29fc1cc8b68bf74bfc0c9c2274f0af84eee
>
> Diff: https://reviews.apache.org/r/50415/diff/
>
>
> Testing
> -------
>
> cmake && make protobuf-2.6.1-java
> check if 3rdparty/protobuf-2.6.1/src/protobuf-2.6.1-java/java/target/protobuf-java-2.6.1.jar
>
>
> Thanks,
>
> Srinivas Brahmaroutu
>
>
Re: Review Request 50415: Added build step to build Java Protobuf.
Posted by Srinivas Brahmaroutu <sr...@us.ibm.com>.
> On Dec. 8, 2016, 8:59 p.m., Alex Clemmer wrote:
> > 3rdparty/CMakeLists.txt, line 311
> > <https://reviews.apache.org/r/50415/diff/6/?file=1579858#file1579858line311>
> >
> > I must be missing something, because I don't quite see why it's necessary to copy `protoc` over to the Java root?
the build step in the source code requires protoc at relative location ../src/protoc
> On Dec. 8, 2016, 8:59 p.m., Alex Clemmer wrote:
> > 3rdparty/CMakeLists.txt, line 347
> > <https://reviews.apache.org/r/50415/diff/6/?file=1579858#file1579858line347>
> >
> > If I'm understanding this correctly, this will download a fresh copy of the protobuf tarball, and then build only the Java parts of it.
> >
> > Since we're already downloading this for the normal Protobuf build, it seems like it would be easier to get rid of the `ExternalProject_Add` call and simply append the java config, build, and install commands to the protobuf config, build, and install commands. For example:
> >
> > ```
> > set(PROTOBUF_BUILD_CMD ${PROTOBUF_BUILD_CMD} ${PROTOBUF_JAVA_BUILD_CMD})
> > ```
> >
> > This should cause the normal protobuf build to build the Java stuff as well, no?
I prefer to build the jar as a step for Java and I will add another step for Python. Hope this works.
- Srinivas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50415/#review158565
-----------------------------------------------------------
On Dec. 8, 2016, 11:04 p.m., Srinivas Brahmaroutu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50415/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 11:04 p.m.)
>
>
> Review request for mesos, Alex Clemmer and Joseph Wu.
>
>
> Bugs: MESOS-5902
> https://issues.apache.org/jira/browse/MESOS-5902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added config, build and install steps for the External project
> protobuf_java.
> 'make probuf_java' produces the
> ${PROTOBUF_JAVA_ROOT}/java/target/protobuf-java-2.6.1.jar
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt a340b29fc1cc8b68bf74bfc0c9c2274f0af84eee
>
> Diff: https://reviews.apache.org/r/50415/diff/
>
>
> Testing
> -------
>
> cmake && make protobuf-2.6.1-java
> check if 3rdparty/protobuf-2.6.1/src/protobuf-2.6.1-java/java/target/protobuf-java-2.6.1.jar
>
>
> Thanks,
>
> Srinivas Brahmaroutu
>
>
Re: Review Request 50415: Added build step to build Java Protobuf.
Posted by Alex Clemmer <cl...@gmail.com>.
> On Dec. 8, 2016, 8:59 p.m., Alex Clemmer wrote:
> > 3rdparty/CMakeLists.txt, line 347
> > <https://reviews.apache.org/r/50415/diff/6/?file=1579858#file1579858line347>
> >
> > If I'm understanding this correctly, this will download a fresh copy of the protobuf tarball, and then build only the Java parts of it.
> >
> > Since we're already downloading this for the normal Protobuf build, it seems like it would be easier to get rid of the `ExternalProject_Add` call and simply append the java config, build, and install commands to the protobuf config, build, and install commands. For example:
> >
> > ```
> > set(PROTOBUF_BUILD_CMD ${PROTOBUF_BUILD_CMD} ${PROTOBUF_JAVA_BUILD_CMD})
> > ```
> >
> > This should cause the normal protobuf build to build the Java stuff as well, no?
>
> Srinivas Brahmaroutu wrote:
> I prefer to build the jar as a step for Java and I will add another step for Python. Hope this works.
Can you speak a little more about why you prefer it? I personally would prefer to keep the Java, native, and Python builds of PB all coming from the same source tree just because it's less error-prone.
> On Dec. 8, 2016, 8:59 p.m., Alex Clemmer wrote:
> > 3rdparty/CMakeLists.txt, line 311
> > <https://reviews.apache.org/r/50415/diff/6/?file=1579858#file1579858line311>
> >
> > I must be missing something, because I don't quite see why it's necessary to copy `protoc` over to the Java root?
>
> Srinivas Brahmaroutu wrote:
> the build step in the source code requires protoc at relative location ../src/protoc
Ah, you seem to have removed it now? Also, I suspect that this will work if you build the native code in the same source tree as the Java code. Is that not correct?
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50415/#review158565
-----------------------------------------------------------
On Dec. 10, 2016, 7:18 p.m., Srinivas Brahmaroutu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50415/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 7:18 p.m.)
>
>
> Review request for mesos, Alex Clemmer and Joseph Wu.
>
>
> Bugs: MESOS-5902
> https://issues.apache.org/jira/browse/MESOS-5902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added config, build and install steps for the External project
> protobuf_java.
> 'make probuf_java' produces the
> ${PROTOBUF_JAVA_ROOT}/java/target/protobuf-java-2.6.1.jar
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt a340b29fc1cc8b68bf74bfc0c9c2274f0af84eee
>
> Diff: https://reviews.apache.org/r/50415/diff/
>
>
> Testing
> -------
>
> cmake && make protobuf-2.6.1-java
> check if 3rdparty/protobuf-2.6.1/src/protobuf-2.6.1-java/java/target/protobuf-java-2.6.1.jar
>
>
> Thanks,
>
> Srinivas Brahmaroutu
>
>
Re: Review Request 50415: Added build step to build Java Protobuf.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50415/#review158565
-----------------------------------------------------------
3rdparty/CMakeLists.txt (line 310)
<https://reviews.apache.org/r/50415/#comment229349>
I think we can probably remove this, right? (Also, if you didn't know, these variables are all written down in `build/CMakeCache.txt` when you configure.)
3rdparty/CMakeLists.txt (line 311)
<https://reviews.apache.org/r/50415/#comment229351>
I must be missing something, because I don't quite see why it's necessary to copy `protoc` over to the Java root?
3rdparty/CMakeLists.txt (line 347)
<https://reviews.apache.org/r/50415/#comment229350>
If I'm understanding this correctly, this will download a fresh copy of the protobuf tarball, and then build only the Java parts of it.
Since we're already downloading this for the normal Protobuf build, it seems like it would be easier to get rid of the `ExternalProject_Add` call and simply append the java config, build, and install commands to the protobuf config, build, and install commands. For example:
```
set(PROTOBUF_BUILD_CMD ${PROTOBUF_BUILD_CMD} ${PROTOBUF_JAVA_BUILD_CMD})
```
This should cause the normal protobuf build to build the Java stuff as well, no?
- Alex Clemmer
On Dec. 8, 2016, 6:19 p.m., Srinivas Brahmaroutu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50415/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 6:19 p.m.)
>
>
> Review request for mesos, Alex Clemmer and Joseph Wu.
>
>
> Bugs: MESOS-5902
> https://issues.apache.org/jira/browse/MESOS-5902
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added config, build and install steps for the External project
> protobuf_java.
> 'make probuf_java' produces the
> ${PROTOBUF_JAVA_ROOT}/java/target/protobuf-java-2.6.1.jar
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt a340b29fc1cc8b68bf74bfc0c9c2274f0af84eee
>
> Diff: https://reviews.apache.org/r/50415/diff/
>
>
> Testing
> -------
>
> cmake && make protobuf-2.6.1-java
> check if 3rdparty/protobuf-2.6.1/src/protobuf-2.6.1-java/java/target/protobuf-java-2.6.1.jar
>
>
> Thanks,
>
> Srinivas Brahmaroutu
>
>