Migrate to SDK.v3 - protocol.v2 with decode #84

bzz · 2019-05-28T16:38:02Z

Partially addresses #83

new API is under org.bblfsh.client.v2
new SDK and libraste is used
decode() and root() are implemented
add CI profile only with things that are expected to pass at current stage
add 2 TODOs base on feedback
enable test for the latest bblfshd/sdk release
fix CI v2 profile
Right now failing with java.lang.IllegalStateException: Linux build failed -> fatal error: libuast.h: No such file or directory
API visibility in native lib
~~A bit improved tests of .decode()~~
As C++ API does not expose .Kind() for nodes, this can be done later, after .load() is implemented.

Main CI is expected to fail due to still missing native API that will be added in separate PRs.

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

Many tests are Ignored or are expected to fail, due to rest of the native API not implemented yet. Signed-off-by: Alexander Bezzubov <bzz@apache.org>

dennwc

First round is done! I don't understand most of the JNI code, but at least the Scala code is clear. Not marked as approved because there are still few changes needed (like mentioned in the FIXME).

build.sbt

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc

src/main/scala/org/bblfsh/client/cli/ScalaClientCLI.scala

src/main/scala/org/bblfsh/client/v2/BblfshClient.scala

src/main/scala/org/bblfsh/client/v2/Node.scala

src/test/scala/org/bblfsh/client/v2/BblfshClientClose.scala

src/test/scala/org/bblfsh/client/v2/BblfshClientParseTest.scala

creachadair

A few comments in passing.

build.sbt

src/main/native/jni_handle.h

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc

src/main/scala/org/bblfsh/client/v2/BblfshClient.scala

creachadair · 2019-05-29T01:29:56Z

First round is done! I don't understand most of the JNI code, but at least the Scala code is clear. Not marked as approved because there are still few changes needed (like mentioned in the FIXME).

It's funny, because I was just feeling much more comfortable with the C++ portion. Which I suppose says something about my poor life-choices. 😀

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

It also adds a temp job that will be kept updated and is expected to pass for bblfsh#83 and track it's progress. Signed-off-by: Alexander Bezzubov <bzz@apache.org>

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

bzz · 2019-05-29T15:18:09Z

CI is expected to have 1 profile that is passing now. Initial feedback addressed.

2 things that are left as are waiting for the feedback:

API visibility improvement on the native part
a bit more sophisticated tests for decode()

And a CI fix for fatal error: libuast.h: No such file or directory. PR description updated to include those.

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

Only tests that are passing on V2 are currently enabled for every PR. Signed-off-by: Alexander Bezzubov <bzz@apache.org>

bzz · 2019-06-03T14:40:53Z

@creachadair @dennwc all feedback was addressed, CI v2-enabled profile is green and it's ready to

creachadair

Just a few questions left.

.clang-format

creachadair · 2019-06-03T16:46:03Z

src/main/scala/org/bblfsh/client/v2/BblfshClient.scala

+object BblfshClient {
+  val DEFAULT_MAX_MSG_SIZE = 100 * 1024 * 1024 // bytes
+
+  val PreOrder      = 0


These don't seem to be used anywhere—are they mean to support the iterator implementation that is coming later?

Yes, exactly - this is part of the old code that was moved forward and going to be used for iterator implementation.

creachadair · 2019-06-03T16:52:38Z

src/main/native/jni_utils.h

+extern const char *CLS_CTX;
+
+JNIEnv *getJNIEnv();
+jobject NewJavaObject(JNIEnv *, const char *, const char *, ...);


I'm not sure I follow the naming convention in use here. Google style is MixedCaseLeadingCaps for classes and functions (with a carve-out for class accessors to follow snake_case style). But we seem to be mixing that and conventional camel.

(I'm not wedded to Google style, I only mentioned that because you included a clang-format config based on that style)

This is an old code, I belive it follow the same that was used in libuast.
TBH Google one from above was just placed randomly, to have at least something, and mostly because of 2-space indentation.

I'm not settled on this one either, but I think it's totally fine to use almost whatever in a the well-localized JNI wrappers themselves.

Will be happy to refactor it, if needed, and add a better formatter config to all the projects later on, as soon as we have something functional in scala-client. For now, I'm mostly not very worried about it because of the limited scope and no expectations for this native piece to grow.

Yeah, this doesn't need to be addressed right now, I just wanted to make a note of it while we're paying attention to this code. 🙂

build.sbt

.travis.yml

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

bzz · 2019-06-03T19:45:29Z

Just a few questions left.

@creachadair I tried to answer all of those and address the feedback, let me know what you think!
And it's ready for another round.

bzz added 3 commits May 25, 2019 05:44

v2 setup: new SDK, new API package, no native part yet

a48d373

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

v2 setup: new tests

fcc5112

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

v2 setup: new libuast, new native JNI

80a7a9e

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

bzz self-assigned this May 28, 2019

v2: native .decode() and .root() implementations

d7cd8f0

Many tests are Ignored or are expected to fail, due to rest of the native API not implemented yet. Signed-off-by: Alexander Bezzubov <bzz@apache.org>

bzz force-pushed the migrate-to-sdk-v3 branch from e43991f to d7cd8f0 Compare May 28, 2019 16:40

dennwc reviewed May 28, 2019

View reviewed changes

creachadair reviewed May 29, 2019

View reviewed changes

bzz mentioned this pull request May 29, 2019

Migrate scala-client to SDK.v3 + libuast.v3 #83

Closed

17 tasks

bzz added 4 commits May 29, 2019 16:03

v2: enable all tests \w bblfshd >v2.14.0

8f249f7

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

ci: refactor to multiple jobs

5761a11

It also adds a temp job that will be kept updated and is expected to pass for bblfsh#83 and track it's progress. Signed-off-by: Alexander Bezzubov <bzz@apache.org>

v2: add local clang format configuration for C/C++

198ee00

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

v2: first round of code review addressed

2c05d07

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

bzz force-pushed the migrate-to-sdk-v3 branch from dd1a115 to 2c05d07 Compare May 29, 2019 15:05

bzz force-pushed the migrate-to-sdk-v3 branch 9 times, most recently from 64f96a5 to 18a67bc Compare May 31, 2019 08:54

ci: move verbose ouput

7c9d362

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

bzz force-pushed the migrate-to-sdk-v3 branch from 18a67bc to 7c9d362 Compare May 31, 2019 15:21

bzz added 2 commits May 31, 2019 17:32

native: refactoring, move to anonymous namespace

26bd96a

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

build: clean downloaded artefacts on 'sbt clean'

b12b1ab

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

bzz force-pushed the migrate-to-sdk-v3 branch from 377d8b1 to 9e98235 Compare June 1, 2019 19:37

bzz force-pushed the migrate-to-sdk-v3 branch 13 times, most recently from 9a7ff1f to bde14f9 Compare June 3, 2019 14:19

ci: only v2 tests, more debug output

cbb1b3e

Only tests that are passing on V2 are currently enabled for every PR. Signed-off-by: Alexander Bezzubov <bzz@apache.org>

bzz force-pushed the migrate-to-sdk-v3 branch from bde14f9 to cbb1b3e Compare June 3, 2019 14:36

dennwc approved these changes Jun 3, 2019

View reviewed changes

creachadair reviewed Jun 3, 2019

View reviewed changes

This was referenced Jun 3, 2019

Improve the CI #87

Closed

Restructure build #88

Closed

code review feedback

f023d7c

Signed-off-by: Alexander Bezzubov <bzz@apache.org>

creachadair approved these changes Jun 3, 2019

View reviewed changes

bzz merged commit bc7f973 into bblfsh:master Jun 3, 2019

bzz deleted the migrate-to-sdk-v3 branch June 3, 2019 20:28

bzz mentioned this pull request Jun 6, 2019

encode() and a stub for the load() #89

Merged

bzz mentioned this pull request Jul 10, 2019

v2: expose custom gRPC connection timeouts #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to SDK.v3 - protocol.v2 with decode #84

Migrate to SDK.v3 - protocol.v2 with decode #84

bzz commented May 28, 2019 •

edited

Loading

dennwc left a comment

creachadair left a comment

creachadair commented May 29, 2019

bzz commented May 29, 2019 •

edited

Loading

bzz commented Jun 3, 2019

creachadair left a comment

creachadair Jun 3, 2019

bzz Jun 3, 2019

creachadair Jun 3, 2019

bzz Jun 3, 2019 •

edited

Loading

creachadair Jun 3, 2019

bzz commented Jun 3, 2019

Migrate to SDK.v3 - protocol.v2 with decode #84

Migrate to SDK.v3 - protocol.v2 with decode #84

Conversation

bzz commented May 28, 2019 • edited Loading

dennwc left a comment

Choose a reason for hiding this comment

creachadair left a comment

Choose a reason for hiding this comment

creachadair commented May 29, 2019

bzz commented May 29, 2019 • edited Loading

bzz commented Jun 3, 2019

creachadair left a comment

Choose a reason for hiding this comment

creachadair Jun 3, 2019

Choose a reason for hiding this comment

bzz Jun 3, 2019

Choose a reason for hiding this comment

creachadair Jun 3, 2019

Choose a reason for hiding this comment

bzz Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

creachadair Jun 3, 2019

Choose a reason for hiding this comment

bzz commented Jun 3, 2019

bzz commented May 28, 2019 •

edited

Loading

bzz commented May 29, 2019 •

edited

Loading

bzz Jun 3, 2019 •

edited

Loading