kaitai-io / kaitai_struct

Kaitai Struct: declarative language to generate binary data parsers in C++ / C# / Go / Java / JavaScript / Lua / Nim / Perl / PHP / Python / Ruby
https://kaitai.io
4.04k stars 199 forks source link

Mix Maven + KSC #1120

Closed mstup closed 3 months ago

mstup commented 3 months ago

I tried it to combine kaitai_struct_compiler 0.10 with my Maven based Java project.

In project I have ksy-file and want to compile it each time before the project Java code is being compiled. For this purpose in pom.xml I configured exec-maven-plugin to execute on generate-sources phase. It's used quite often to generate some kind of code from DSL-files.

I know about two possible ways to compile ksy-file:

  1. Use externally installed _kaitai_structcompiler
  2. Include _kaitai_structcompiler jar-files into project and call JavaMain.main method.

Option 1 works perfectly and no questions about this, but it has drawback - needs manually install compiler.

Option 2 when I build project, it fails with error for some JavaMain internal code.

java.lang.NullPointerException: Cannot invoke "java.net.URL.getPath()" because the return value of "java.security.CodeSource.getLocation()" is null at io.kaitai.struct.JavaMain$.homePath (JavaMain.scala:209) at io.kaitai.struct.JavaMain$.main (JavaMain.scala:246) at io.kaitai.struct.JavaMain.main (JavaMain.scala) at org.codehaus.mojo.exec.ExecJavaMojo.doMain (ExecJavaMojo.java:385) at org.codehaus.mojo.exec.ExecJavaMojo.doExec (ExecJavaMojo.java:374) at org.codehaus.mojo.exec.ExecJavaMojo.lambda$execute$0 (ExecJavaMojo.java:296) at java.lang.Thread.run (Thread.java:840)

I attached dummy project to reproduce the problem. To reproduce this issue needs to have Maven 3.8+ installed, enter the project root and run command mvn clean package

foo-bar.zip

BTW: I run it on Fedora 38

GreyCat commented 3 months ago

If your aim is to build .ksy in compile-time, why not use something like https://github.com/valery1707/kaitai-maven-plugin?

mstup commented 3 months ago

Oh, I just didn't know about existing of such Maven plugin. Thanks.

mstup commented 3 months ago

I looked over the kaitai-maven-plugin and don't see a big difference with the case when exec-maven-plugin is calling external preinstalled compiler. With this plugin just don't need to install compiler. It can works as workaround. I would ask somebody to look at exception that I reported because it's pure compiler code. Looks like there need to use some other approach in initialization of homePath variable in JavaMain class.

BTW: What problem an author tried to resolve in this code? I think this code will work always:

Class javaMainClass = io.kaitai.struct.JavaMain.class;
String mainClassPath = javaMainClass.getClassLoader()
        .getResource(javaMainClass.getName().replace('.', '/') + ".class").getPath();
String jarPath = mainClassPath.substring("file:".length(), mainClassPath.lastIndexOf("!/"));
File parentFolder = new File(jarPath).getParentFile();

This code could looks a bit ugly but Java use the same approach to work with file URI schema. Take a look at java.net.JarURLConnection.parseSpecs(URL)

GreyCat commented 3 months ago

I believe what we've tried to solve there was detecting home path based on a lot of JVM installation scenarios, which all give quite different results.

Your scenario might need another special case, or, if you're confident that new approach you're suggesting can replace all that complicated logic — absolutely happy to.

If you want to proceed, I want to suggest the following steps:

  1. Reproduce the problem with modern compiler and figure out where the issue is. Right now what you're showing is pointing to JavaMain.scala:209, which is a comment line in modern codebase. Let's at least understand what exactly is blowing up.
  2. Implement that fix as you suggest it in Scala code.
  3. Test it (either manually, as described in these comments, or, better yet, create a way to automatically test all these permutations).
  4. Prepare a PR with the changes you suggest.
mstup commented 3 months ago

The problem is line 213. The method getLocation result is NULL and that cause the problem.

This code snippet that I proposed in Java is just an idea and should work for any operation system, but I don't know what edge cases you been faced with.

I'm sorry, really want to help with this but have no idea how Scala work and how to test it.

generalmimon commented 3 months ago

@GreyCat:

I believe what we've tried to solve there was detecting home path based on a lot of JVM installation scenarios

The docstring sums up the purpose quite well (JavaMain.scala:174-177):

  /**
    * Insanely hacky method that relies on some JVM black magic to get
    * application "home". From that, we can check if it has a "formats" subdir,
    * and if it does, consider that an additional default ksy imports path.

See https://github.com/kaitai-io/kaitai_struct/issues/136 - universal .zip build comes bundled with the format library (KSF) in the formats/ subdir, so you can treat the formats in KSF as part of the "Kaitai standard library" and just use imports: - /common/bcd without caring where the bcd.ksy spec comes from. However, this obviously requires the compiler to resolve its location, more specifically the location of the .jar file it is running from so that it can then add ../formats to search paths for absolute imports.

I guess this feature fundamentally makes no sense in the scenario that @mstup is trying to do because most likely there will be no formats dir available anyway. So I suppose the fix would be to just silently (optionally with some warning, if it even makes any sense) ignore the failure of the "application home path resolution" and just move on.

BTW, https://github.com/kaitai-io/kaitai_struct_gui also calls KSC as a library via Java API, but it sidesteps this issue (perhaps unknowingly) by not calling JavaMain.main but instead calling some "lower-level" methods of KSC (VisualizerPanel.java:129-157), in which case the problematic code does not run. So this might be something to check out, @mstup, you can use this at least as a workaround (but granted, the configuration probably won't be as convenient as with the command line options).

GreyCat commented 3 months ago

@generalmimon In the end of the day, I believe the problem that @mstup demonstrated is very real: given getLocation can return null, we should wrap it in Option, something like:

-    val fStr = classOf[JavaMain].getProtectionDomain.getCodeSource.getLocation.getPath
+    val fStr = Option(classOf[JavaMain].getProtectionDomain.getCodeSource.getLocation).map(getPath)

... and treat it as Option down the road.

@mstup Can you clarify if you'll need formats dir autodetection in your situation? Do you even need full command line parsing facility, or you can bypass that?

mstup commented 3 months ago

@GreyCat No, I don't need formats dir in my case, but even if I would like to have this folder I can specify location in ksc compilator parameter --import-path

generalmimon commented 3 months ago

Well, what I was trying to say is that if you use the kaitai-struct-compiler_2.12 package from Maven Central, there is no "formats" dir anywhere. It is not included in this distribution. So any "formats dir autodetection" is pointless in this case.

mstup commented 3 months ago

@generalmimon exactly @GreyCat idea with using Optional looks like a very promising solution.

BTW: This code in Java will work always and do not depends is it Maven or any other case. I'm talking about following possible cases:

GreyCat commented 3 months ago

@mstup I went ahead and tried wrapping homePath into various Option-based safety precautions. Please check out https://github.com/kaitai-io/kaitai_struct_compiler/commit/83df9977348b108fb74e649df39b91ce5cfc465e — see if this works for you?

It should complain in the logs, but guarantees to return None if nothing was really found for whatever reason, instead of exploding with NPE.

mstup commented 3 months ago

@GreyCat This PR looks good.

I manually substituted code from master (0.11-SNAPSHOT) and it works fine now. In console printed message (as it designed in the code):

home path: unable to run getLocation(), got null

GreyCat commented 3 months ago

I assume we can consider this resolved then :)