Initial implementation of Java bindings

[jrhea]

This PR implements bindings for Java. All tests pass on OSX and debian:lastest.

Punchlist

Code:

• evmc_host_interface jni
• evmc_host_interface java
• evmc_vm interface jni
• evmc_vm interface java

Tests:

• Init
• EVMC_CREATE
• return_address
• counter
• return_block_number
• save_return_block_number
• make_a_call

Build

• integrate into CI

Other

• README.md to orient people who want to use java bindings

Resolves #137
helps with ethereum/ethereumj#1025

[atoulme]

Looks good. Got cosmetic feedback.

[mkalinin]

From my standpoint there should be a pair of abstractions:

• Evmc as a general interface to evmc
• EvmcVM which is instantiated via Evmc.create(String filename) and effectively a VM instance

It looks organic to me and allows user to instantiate several VM instances at a time.

EvmcVM could implement AutoCloseable interface to utilize try-with-resources pattern.

[chfast]

I enabled building of external PR. If you push more commits the CI jobs should run correctly.

[chfast]

You can also check #171.

[jrhea]

From my standpoint there should be a pair of abstractions:

• Evmc as a general interface to evmc
• EvmcVM which is instantiated via Evmc.create(String filename) and effectively a VM instance

It looks organic to me and allows user to instantiate several VM instances at a time.

EvmcVM could implement AutoCloseable interface to utilize try-with-resources pattern.

That does sound nice, but realistically why I would want to call Evmc.create(filename) multiple times. It's a little awkward considering that I will only load the shared object once. @chfast pointed out in another comment that there is typically one long lived vm that can handle multiple concurrent calls. That being said, I have some work to do to make sure everything is thread safe.

[chfast]

From my standpoint there should be a pair of abstractions:

• Evmc as a general interface to evmc
• EvmcVM which is instantiated via Evmc.create(String filename) and effectively a VM instance

It looks organic to me and allows user to instantiate several VM instances at a time.
EvmcVM could implement AutoCloseable interface to utilize try-with-resources pattern.

That does sound nice, but realistically why I would want to call Evmc.create(filename) multiple times. It's a little awkward considering that I will only load the shared object once. @chfast pointed out in another comment that there is typically one long lived vm that can handle multiple concurrent calls. That being said, I have some work to do to make sure everything is thread safe.

We are using this abstraction in all language bindings. This is mostly to make sure the VM instance is automatically released and also it wraps execution result object to also make sure it is properly released. But still the EvmcVM stays a singleton in most cases.

I still have to review this PR. I also don't think introducing this abstraction in the first iteration is crucial. I'm also guessing your goal is to consume other VM, not to export Java one.

[mkalinin]

but realistically why I would want to call Evmc.create(filename) multiple times.

Fair point. My abstraction model came out of my understanding of evmc design but it looks overcomplicated from user's perspective. I'd then keep EvmcVM class with create or getInstance factory method which loads evmc lib if it's not yet loaded and creates an instance of VM with filename specified by user. This instance would keep track of own vm_handle and dispose it when it's not needed anymore (explicitly by a user or by try-with-resources pattern).

From my experience having a deal with statically stored state is not that convenient in some cases. What if you want to run two VM instances within the same JVM process? For example, to test whether two different implementations are conformant with each other; however, this is hardly the case for this bindings and not that good example.

To conclude, IMO it's better to get rid of statics, it would give more flexibility to the user. But I agree that this is not necessary at the moment and could be done on demand.

and also it wraps execution result object to also make sure it is properly released

Good point. evmc_result should be disposed when it's not needed likewise VM itself. Is there any other resources that requires explicit disposal?

Also, I think it worth to mention in the head of HostContext interface that its implementation should be thread-safe.

[jrhea]

@mkalinin, @chfast, @atoulme great discussion. Thanks to for the guidance. Tbh, I am still getting a feel for how this will be used so your input here is invaluable. I want to point out that the test coverage is abysmal at this point. That being said, it could potentially be merged assuming we document it as in progress (once @chfast reviews of course). An iterative approach might make it easier to review going fwd. 2k line PRs are incredibly difficult to review - sorry about that.

[jrhea]

We are using this abstraction in all language bindings. This is mostly to make sure the VM instance is automatically released and also it wraps execution result object to also make sure it is properly released. But still the EvmcVM stays a singleton in most cases.

Thanks for this and good point about ensuring the result object is properly released. I have some work to do on this front. I will keep it static (for now) and change it to the abstraction that you, @mkalinin and @atoulme all suggested.

I still have to review this PR. I also don't think introducing this abstraction in the first iteration is crucial.

I can stop now and wait for your review, or I can continue work. I think the size of the PR is starting to reach the point where it becomes difficult to review.

I'm also guessing your goal is to consume other VM, not to export Java one.

Yessir the goal is to consume another VM

[jrhea]

I'd then keep EvmcVM class with create or getInstance factory method which loads evmc lib if it's not yet loaded and creates an instance of VM with filename specified by user. This instance would keep track of own vm_handle and dispose it when it's not needed anymore (explicitly by a user or by try-with-resources pattern).

This was very well said and totally doable.

From my experience having a deal with statically stored state is not that convenient in some cases.

100% agree here

Good point. evmc_result should be disposed when it's not needed likewise VM itself. Is there any other resources that requires explicit disposal?

Good question.

Also, I think it worth to mention in the head of HostContext interface that its implementation should be thread-safe.

agreed. i am open to suggestions

[chfast]

Do we need gradle-wrapper.jar and gradlew?

[chfast]

I did a quick review.

I've found some issues with the C code, most of them should be fixed before merging.

I'm also concern a bit about the build process. Can we actually live without adding gradle wrapper and assume gradle is installed? Is there also a concern about the gradle version?

[chfast]

Is this common to provide build scripts for C compilation? If yes, then I think JNI should be pulled to main cmake script and build with it. I can handle that later.

[jrhea]

that'd be great if you could tie it into the main script. thanks!

[axic]

Also as a measure before merging, can you enable C formatting checks?

In https://github.com/ethereum/evmc/blob/master/circle.yml#L67 change:

-          find examples include lib test -name '*.hpp' -o -name '*.cpp' -o -name '*.h' -o -name '*.c' | xargs clang-format -i
+          find bindings/java examples include lib test -name '*.hpp' -o -name '*.cpp' -o -name '*.h' -o -name '*.c' | xargs clang-format -i

Also useful to install clang-format and run (clang-format -i <filename>) locally on your source so it can fix it.

[axic]

@jrhea and let me know when the PR is ready for a final review.

[jrhea]

@jrhea and let me know when the PR is ready for a final review.

I believe it is ready for a final review

[axic]

From my side this looks okay for a merge (and adding other improvements later).

@jrhea do you want to squash these into a single commit (once @chfast gives his final review)?

[jrhea]

From my side this looks okay for a merge (and adding other improvements later).

@jrhea do you want to squash these into a single commit (once @chfast gives his final review)?

whatever your normal procedure is works for me.

[chfast]

From my side this looks okay for a merge (and adding other improvements later).
@jrhea do you want to squash these into a single commit (once @chfast gives his final review)?

whatever your normal procedure is works for me.

This is not normal procedure, but because it only adds new stuff and has a reformat commit in the middle, I prefer to squash it.

Also, please add a changelog entry.

I actually have some number of suggestions for improvements, but it will be better to merge it as it is and follow up with future PRs.

[jrhea]

This is not normal procedure, but because it only adds new stuff and has a reformat commit in the middle, I prefer to squash it.

ok that's fine, then squash it. I only added the reformat commit bc i was asked to.

Also, please add a changelog entry.

done.

I actually have some number of suggestions for improvements, but it will be better to merge it as it is and follow up with future PRs.

ditto...it was a good first pass at least.

[axic]

Thanks @jrhea for the work and @mkalinin @atoulme for the reviews!

[jrhea]

Thanks @jrhea for the work and @mkalinin @atoulme for the reviews!

sure thing. it was fun. thanks all for the reviews