Skip to content

Add control buffers to java build#2

Merged
wfernandes merged 1 commit intocloudfoundry:masterfrom
dsyer:feature/java-control
Sep 9, 2015
Merged

Add control buffers to java build#2
wfernandes merged 1 commit intocloudfoundry:masterfrom
dsyer:feature/java-control

Conversation

@dsyer
Copy link
Copy Markdown

@dsyer dsyer commented May 8, 2015

Also make Java build work without an explicit target directory and
add instructions in README.

@cf-gitbot
Copy link
Copy Markdown

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/94198358.

README.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this change; we'd like to keep our final newlines per UNIX standards.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't tell what the change is from the github UI, and I don't know what the "standard" is. I'm removing the trailing newline, hoping that was what you meant.

Also make Java build work without an explicit target directory and
add instructions in README.
@dsyer dsyer force-pushed the feature/java-control branch from eaad3b8 to 85e21a1 Compare May 11, 2015 07:58
@jmtuley
Copy link
Copy Markdown
Contributor

jmtuley commented May 11, 2015

@dsyer so it would seem that the issue with generating the Java tooling is that we import bits from gogoprotobuf, so you have to download that anyway. Am I reading that correctly?

If that's the case, then maybe we'll spend some time after Summit figuring out how to only require that library when generating Go code; it doesn't seem to me to be good policy to force Java devs through lots of hoops that have no effect on them.

@dsyer
Copy link
Copy Markdown
Author

dsyer commented May 12, 2015

That would be good (not requiring gogoprotobuf) if it's an option, but I don't really understand what it does. I have old versions of .java files generated a while back and they still work with no reference to that stuff, so there might be a way to do that and for the generated code to still work. But then again, if they are needed for Go, why would they not be needed for Java users (it looks like it's probably just another library for Java users, but I haven't tried to make it work)?

@jmtuley
Copy link
Copy Markdown
Contributor

jmtuley commented May 12, 2015

Gogoprotobuf is the tool we use to generate Go code. The options/extensions
related to it are solely for its own use. Java users shouldn't be affected.

On Tuesday, May 12, 2015, Dave Syer notifications@github.com wrote:

That would be good (not requiring gogoprotobuf) if it's an option, but I
don't really understand what it does. I have old versions of .java files
generated a while back and they still work with no reference to that stuff,
so there might be a way to do that and for the generated code to still
work. But then again, if they are needed for Go, why would they not be
needed for Java users (it looks like it's probably just another library for
Java users, but I haven't tried to make it work)?


Reply to this email directly or view it on GitHub
#2 (comment)
.

– John Tuley

@roxtar
Copy link
Copy Markdown
Contributor

roxtar commented May 15, 2015

@dsyer We've removed the dependency of the Java generator on the Go imports. We'd like you to apply the following diff and commit it to the PR; that way, when we merge it in, we'll be sure that Java users will benefit from your changes and not need to install Go and gogo/protobuf first.

diff --git a/README.md b/README.md
index f70e0b5..3eae60e 100644
--- a/README.md
+++ b/README.md
@@ -31,10 +31,7 @@ Please see the following for detailed descriptions of each type:
    ```

 ### Java
-
-1. Build the go code first (see above) so that all the imports are available
-
-2. Generate the Java code (optionally providing a target path as a directory)
+1. Generate the Java code (optionally providing a target path as a directory)
    ```
    ./generate-java.sh [TARGET_PATH]
    ```
diff --git a/generate-java.sh b/generate-java.sh
index 6afdede..a858395 100755
--- a/generate-java.sh
+++ b/generate-java.sh
@@ -19,11 +19,11 @@ fi

 pushd events
 mkdir -p $TARGET
-protoc -I=. --java_out=$TARGET --proto_path=$GOPATH/src:$GOPATH/src/github.com/gogo/protobuf/protobuf:. *.proto
+protoc -I=. --java_out=$TARGET *.proto
 popd

 pushd control
 mkdir -p $TARGET
-protoc -I=. --java_out=$TARGET --proto_path=$GOPATH/src:$GOPATH/src/github.com/gogo/protobuf/protobuf:. *.proto
+protoc -I=. --java_out=$TARGET *.proto
 popd

@dsabeti
Copy link
Copy Markdown

dsabeti commented Aug 28, 2015

We have removed the control messages from the dropsonde-protocol. We feel that the README changes in this PR are useful. If you modify the PR to only include those changes, we can merge this in.

@wfernandes wfernandes merged commit 85e21a1 into cloudfoundry:master Sep 9, 2015
@roxtar
Copy link
Copy Markdown
Contributor

roxtar commented Sep 9, 2015

We have merged this PR with only the changes in the README.md and the generate-java.sh script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants