Re: Review Request 50415: Added build step to build Java Protobuf.

[Joris Van Remoortere]

---
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.

[Srinivas Brahmaroutu]

> On Dec. 8, 2016, 8:59 p.m., Alex Clemmer wrote:
> > 3rdparty/CMakeLists.txt, line 347
> > 
> >
> > 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.

[Alex Clemmer]

> On Dec. 8, 2016, 8:59 p.m., Alex Clemmer wrote:
> > 3rdparty/CMakeLists.txt, line 347
> > 
> >
> > 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
> > 
> >
> > 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.

[Srinivas Brahmaroutu]

---
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.

[Srinivas Brahmaroutu]

> On Dec. 8, 2016, 8:59 p.m., Alex Clemmer wrote:
> > 3rdparty/CMakeLists.txt, line 311
> > 
> >
> > 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
> > 
> >
> > 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.

[Srinivas Brahmaroutu]

---
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.

[Alex Clemmer]

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




3rdparty/CMakeLists.txt (line 310)


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)


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)


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
> 
>

Re: Review Request 50415: Added build step to build Java Protobuf.

[Srinivas Brahmaroutu]

---
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.

[Srinivas Brahmaroutu]

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

(Updated Aug. 4, 2016, 8:12 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 build step to build Java Protobuf.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 

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.

[Srinivas Brahmaroutu]

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

(Updated July 29, 2016, 8:12 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 build step to build Java Protobuf.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 

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.

[Srinivas Brahmaroutu]

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

(Updated July 29, 2016, 7:55 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 build step to build Java Protobuf.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 

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.

[Srinivas Brahmaroutu]

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

(Updated July 26, 2016, 7:34 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 build step to build Java Protobuf.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 

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.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50414, 50415]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 25, 2016, 10:31 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50415/
> ---
> 
> (Updated July 25, 2016, 10:31 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
> ---
> 
> This is similar external project build in parallel to normal 
> probuf build for c++. I preferred to use "-java" prefix to 
> separate the project build.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 
> 
> 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.

[Srinivas Brahmaroutu]

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

(Updated July 25, 2016, 10:31 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)
---

This is similar external project build in parallel to normal 
probuf build for c++. I preferred to use "-java" prefix to 
separate the project build.


Diffs
-

  3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 

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

Review Request 50415: Added build step to build Java Protobuf.

[Srinivas Brahmaroutu]

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

Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Added build step to build Java Protobuf.

This is similar external project build in parallel to normal probuf build for 
c++. I preferred to use "-java" prefix to separate the project build.


Diffs
-

  3rdparty/CMakeLists.txt 770a3828a6e0ffaa4f185392fdc1a2152446449d 

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