Conversation
| protected abstract ProtobufTestAdapter createAdapter(Path workdir); | ||
|
|
||
| @Test | ||
| public void shouldInvokeMockSubprocessForExternalPlugin() throws Exception { |
There was a problem hiding this comment.
For reviewers, this test shows the exposed "enabling" functionality
|
Think this is a great start! When I have some time again I'll see if I can hook it into the tooling on my side and see how that goes! Thanks for putting the time into getting this working. |
core-test/src/main/java/io/roastedroot/protobuf4j/test/AbstractCliTest.java
Show resolved
Hide resolved
|
@ascopes have you had the chance to try to integrate those changes into your pipeline, does it helps? |
|
@andreaTP I haven't just yet unfortunately, things have been pretty manic recently. Need to spend some time trying to work out the best way of exposing this such that it still remains totally agnostic. Apologies for the delay. It is still on my radar 🙂 |
|
Thanks for getting back, that's fine, it's enough to hear is on your radar, take your time! |
|
@andreaTP regarding getting a build to try this on, am I able to just fudge the WASM binaries in from this branch into my local Maven repository, or does it require a build from scratch to work? I might have a little time today to fiddle around with some stuff. |
|
You can use the binaries committed on this branch 👍 |
|
Just having a look at this, are we setting the Main-Class Manifest attribute anywhere for the core-v4 package? Thinking we need to override the Maven JAR Plugin in the core-v3 and core-v4 projects to inject the Main-Class attribute into the manifest so the JAR can be executable without users needing to explicitly find out and specify the main class FQDN. (Aware the classpath of the dependency tree is also missing from above but I have logic already in place to be able to compute the appropriate values for this already so that side of this should be fine). |
not yet, it should be easy to do so though, either in the core package or in a dependent "CLI" module. |
|
@andreaTP not blocked per sé as luckily on my side I've already got the ability to inject the entrypoint. Was mostly mentioning in case you wanted to include it on this PR |
|
Good to hear! |
|
Spent a little time looking into this today. I had to add the following to the core-v4 module to get an entrypoint set up (not sure if I was missing where one was defined elsewhere or not): package io.roastedroot.protobuf4j.v4;
import java.nio.file.Path;
import java.util.List;
public final class Main {
private Main() {
throw new UnsupportedOperationException();
}
public static void main(String[] args) {
Protobuf.runProtoc(List.of(args), System.getenv(), Path.of("").toAbsolutePath());
}
}On my side in the Maven plugin, I've had to disable module path lookups as the JAR being generated apparently had an invalid file name when deriving an Automatic Module. I think this is likely able to be fixed by just adding an Automatic-Module-Name manifest entry in next to the Main-Class manifest entry we also discussed. Upon running on my side, I can see that the Java ...but I cannot seem to see anything being output by the application into the For ref, the args passed to protobuf4j on the command line were: ...and the args used to invoke the JVM itself were... I've pushed the code to a branch and the same test can be reproduced via Just want to check I'm not missing anything obvious here? Edit: might've spotted something my side. Will sort that and drop a comment with any further findings! |
|
Okay, update on that. Was a problem on my side it seems. I've pushed a fix to the branch to deal with the new argument propagation correctly and the basic test seems to work. So in conclusion for that, the changes needed to support that side of things would be:
I've also tried to run it with a binary-based gRPC plugin (specifically, protoc-gen-grpc-java) to see if that integration works. Unfortunately, I'm seeing the following output when I do that: Following this, no sources are being generated at all. The JVM invocation looks like the following in this case: Not too sure what is going on there, guessing it is possibly ignoring the I've committed a test project to the branch I mentioned before that can be invoked with |
|
Thanks for getting back @ascopes . I agree that adding Java module definitions is a step forward in this repo, opened #64 to track it. Having the execution failing when attempting to invoke a plugin that is not compiled into this wasm-module is a design choice, see: https://github.com/roastedroot/protobuf4j/pull/54/changes#diff-b1ca38cad24af3b96f6a0186bfa0f16b6fafdc583716ead57604719ce0aad959R118-R128 I do not want to maintain in this repo OS dependent code, nor The All in all, probably the best path forward would be if you roll your own "main" using |
|
I see, okay thanks. Will try and have another look when I get some more time then |
2 participants
This is a step forward to, eventually, enable #30 , I finally took the time to look into it.
I know this is not "exactly" in the direction you envision @ascopes , still it should enable the use case.
Please, @ascopes , verify those changes, and we can move from there.