Fix names of overloaded methods with ref type parameters #139

berkeleybarry · 2023-02-13T22:37:48Z

This pull request is mostly a fix for incorrectly mangling class names used in identifying overloaded methods.

In order to make things run on my Mac, I did a couple of other things. I changed the ASM version for BytecodeUtil. I also set JAVA_HOME for cmake.

pomadchin · 2023-02-22T04:04:59Z

hello @berkeleybarry thanks for a contirbution and a fix!
Could you try to maintain the existing project structure though without intorducing changes to the build.sbt file?

Btw, what's the purpose of those tasty files in the resources directory?

berkeleybarry · 2023-02-22T05:11:58Z

@pomadchin

I added the dependencies to the build.sbt so I can unit test my changes. In what undesirable way does this change the project structure? Is there some other test framework you want me to use? I haven't been part of this project so I find it entirely believable that I am violating some convention that I do not know about.

I used the tasty files for generating headers with a jdk 1.8 javah. I did this so I could see what the correct C function names were. I included the tasty files, and the associated scala source so you can see an example where javah and the plugin did not match. Things will work without these resources.

It might be worth adding an example with overloads and reference type parameters to the test subprojects so you can confirm that a C function with the generated name is actually called when the native method is invoked.

pomadchin

Thanks for a great contribution; the change looks good to me!

But I left a couple of comments to bring this project into the mergable shape; a short summary of the leftovers:

sbt-test project that demonstrates a proper work with the overloaded method, aka integration test
resources folder cleanup
- for tests we only need classes/com/github/sbt/jni/samples/Overloads.class
  - classes/com/github/sbt/jni/samples/Overloads.class is missing
- tasty files removal / move them into the sbt-test project subfolder with a corresponding README
- we don't need C / Java proxy files in the resources folder - they are not used
build.sbt files cleanup (we need the extra salatest dep only)
formatting (run scalafmt)
make CI happy

build.sbt

plugin/src/main/java/com/github/sbt/jni/javah/util/Utils.java

pomadchin · 2023-02-22T12:11:04Z

plugin/src/main/scala/com/github/sbt/jni/build/CMake.scala

@@ -42,6 +42,7 @@ object CMake extends BuildTool with ConfigureMakeInstall {
      s"-DCMAKE_INSTALL_PREFIX:PATH=${target.getAbsolutePath}",
      "-DCMAKE_BUILD_TYPE=Release",
      "-DSBT:BOOLEAN=true",
+      s"-DJAVA_HOME=${scala.sys.props.get("java.home").get}",


What happens in case java.home for some unpredictable reason is empty?

Don't know the answer to that question. I know it did not work on my laptop without providing a java home. The CMake find JNI functionality was lost without it. I think the real solution is to allow this plugin to be configurable to run as is (no JAVA_HOME set), as I modified it, or with a user-specified JAVA_HOME. That should be another PR. I just did enough to get things to work on my laptop.

My comment was that doing .get is unsafe. We can append it to the list of params in case it's present, why not.

This is modified in the update. Is there a way to make it cleaner?

pomadchin · 2023-02-22T12:13:38Z

plugin/src/test/resources/scala/com/github/sbt/jni/samples/Overloads.scala

+class Overloads {
+  @native
+  def doSomething(param: A): B
+  @native
+  def doSomething(param: B): A
+  @native
+  def doSomething(param: Array[C]): A
+  @native
+  def doSomething(parma0: Int, param1: Array[C], param2:B, param3: Double): A
+}


Could we add a full test, similar to others for this proxy along with its JNI component into a separate project into the sbt-test/sbt-jni directory for consistency?

next update

plugin/src/main/scala/com/github/sbt/jni/util/BytecodeUtil.scala

plugin/src/test/scala/com/github/sbt/jni/javah/NativeMethodTests.scala

pomadchin · 2023-02-22T12:45:51Z

plugin/src/test/resources/include/README.md

@@ -0,0 +1 @@
+This directory contains a header file foe the Overloads class that was generated by the javah application of a Java 8 SDK.


I think this does not belong to the resources folder as well as all the related tasty files. But we cat still keep tasty files in a separate folder i.e. in the plugin sbt-test directory in a new directory with a full sample-overloaded project; similar to what we already have.

And we'd need a README to those tasty files and how to deal with them / why we need them.

pomadchin · 2023-02-22T12:59:01Z

plugin/src/main/java/com/github/sbt/jni/javah/util/Utils.java

+        return builder.toString();
+    }
+
+    public static String mangleArguments(String arguments) {


I need to take a closer look and will appreciate some READMEs about the apporach taken with these tasty files, that's quite interesting.

I also checked that this function is backwards compatible on the https://github.com/Glavo/gjavah project tests.
But we still need to ensure sbt-tests of this project are running.

I'm not clear on what you are getting at with your mention of READMEs. Did that line belong to the previous comment?

I mean that if we want to keep these tasty files in the repository we need a direct description of what they are, how to use them to debug, etc.

All this stuff is gone. You can just compile the source in the test code to see it now.

berkeleybarry · 2023-02-22T21:02:51Z

It is not clear to me how much you want me to do here. Who are you expecting me to do the cleanup?
I have time to make the changes but you’ll need to be very clear about what you expect in terms of formatting and organization.

pomadchin · 2023-02-22T21:08:16Z

@berkeleybarry

Hey, I think I am as clear as it's possible; all comments should be resolved before this PR can be merged.

About formatting: this project uses scalafmt; sbt ;scalafmtAll;scalafmtSbt command should work.

The CI should also be green.

pomadchin · 2023-02-22T21:13:15Z

Who are you expecting me to do the cleanup?

This is an open source project, and I am not getting paid to do contributors PR reviews as well. I am happy to help you finishing PR though.

berkeleybarry · 2023-02-22T21:20:39Z

This is what I wanted to know.

…

On Feb 22, 2023, at 1:13 PM, Grigory ***@***.***> wrote: Who are you expecting me to do the cleanup? This is an open source project, and I am not getting paid to do contributors PR reviews as well. I am happy to help you finishing PR though. — Reply to this email directly, view it on GitHub <#139 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAEEQ4RAGVOJAIKI3M5SWDWYZ6PLANCNFSM6AAAAAAU22XM3U>. You are receiving this because you were mentioned.

pomadchin · 2023-02-23T03:53:31Z

plugin/src/main/scala/com/github/sbt/jni/build/CMake.scala

+    override def configure(target: File) = cmakeProcess(
      // disable producing versioned library files, not needed for fat jars
-      val args = Seq(s"-DCMAKE_INSTALL_PREFIX:PATH=${target.getAbsolutePath}", "-DCMAKE_BUILD_TYPE=Release", "-DSBT:BOOLEAN=true") ++ scala.sys.props
-        .get("java.home")
-        .toSeq
-        .map(jh => s"-DJAVA_HOME=${jh}") ++ Seq(cmakeVersion.toString, baseDirectory.getAbsolutePath)
-      cmakeProcess(args: _*)
-    }
+      s"-DCMAKE_INSTALL_PREFIX:PATH=${target.getAbsolutePath}",
+      "-DCMAKE_BUILD_TYPE=Release",
+      "-DSBT:BOOLEAN=true",
+      cmakeVersion.toString,
+      baseDirectory.getAbsolutePath
+    )


@berkeleybarry sadly I had to rollback this change. It looks like this or should be made somehow configurable for the client in a separate PR or smth else 🤔
CI definitely doesn't work with the change.
I searched for a bit, and it looks like it is not a regular practice.

May be setting an env variable may work for you?

berkeleybarry · 2023-02-23T03:59:05Z

What do you want to do about things running on macOS in the tests but not on my laptop (A several week old mac, probably a different cmake version, probably different jdk versions)? This is why I thought it might be wise to make -DJAVA_HOME a configuration option. You can always force it to a value that works. I can’t see enough to suggest another solution that works for all the different environments because I have limited information about the CI builds. Before I started messing with code I tried exporting JAVA_HOME. That did not seem to work. That may depend on how you launch sbt. I was going to write this test when the CI stuff was working for me. I suggest you pass an array of String and make sure that works. What do you want to do about the ASM version?

…

On Feb 22, 2023, at 7:53 PM, Grigory ***@***.***> wrote: @pomadchin commented on this pull request. In plugin/src/main/scala/com/github/sbt/jni/build/CMake.scala <#139 (comment)>: > + override def configure(target: File) = cmakeProcess( // disable producing versioned library files, not needed for fat jars - val args = Seq(s"-DCMAKE_INSTALL_PREFIX:PATH=${target.getAbsolutePath}", "-DCMAKE_BUILD_TYPE=Release", "-DSBT:BOOLEAN=true") ++ scala.sys.props - .get("java.home") - .toSeq - .map(jh => s"-DJAVA_HOME=${jh}") ++ Seq(cmakeVersion.toString, baseDirectory.getAbsolutePath) - cmakeProcess(args: _*) - } + s"-DCMAKE_INSTALL_PREFIX:PATH=${target.getAbsolutePath}", + "-DCMAKE_BUILD_TYPE=Release", + "-DSBT:BOOLEAN=true", + cmakeVersion.toString, + baseDirectory.getAbsolutePath + ) @berkeleybarry <https://github.com/berkeleybarry> sadly I had to rollback this change. It looks like this or should be made somehow configurable for the client in a separate PR or smth else 🤔 CI definitely doesn't work with the change. I searched for a bit, and it looks like it is not a regular practice. May be setting an env variable may work for you? — Reply to this email directly, view it on GitHub <#139 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAEEQ77J6QCUEMKN4GC5VTWY3NMPANCNFSM6AAAAAAU22XM3U>. You are receiving this because you were mentioned.

pomadchin · 2023-02-23T04:08:56Z

@berkeleybarry if you could provide more about the underlying system you have it will be much easier to debug it. I run tests on the m1 mac, with azul jdk 1.8 installed via sdkman.
I also created a follow up issue to make build tools params configurable / overridable: #140

The only reason I had to rollback it is that sbt-tests fail in CI, that means that it may fail in other systems as well. If you can find a different low hanging solution feel free to push it!

I'll pass the array of strings as well, but I was definitely able to reproduce the incorrect methods generation even with the current overrides project contents. It is a really nice catch and fix, thanks!

berkeleybarry · 2023-02-23T04:15:03Z

I don't feel bad about the java.home thing not working everywhere. I did it to solve my problem. Initially, I didn't consider it as a general solution. Once you suggested trying it as a general solution, I thought it was worth a try.

The ASM thing scares me in a similar way. At least that seems to be working where it has been tested.

Adding an additional build tool arguments parameter, Seq[String], seems like an easy and flexible way to solve my problem.

berkeleybarry · 2023-02-23T04:29:00Z

For sbt, I'm running the bt shell inside IntelliJ or launching the same sbt from the command line. This is an example of how intellij launches sbt.

/Library/Java/JavaVirtualMachines/temurin-17.jdk/Contents/Home/bin/java -server -Xmx1536M -Dsbt.supershell=false -Didea.managed=true -Dfile.encoding=UTF-8 "-Didea.installation.dir=/Applications/IntelliJ IDEA.app/Contents" -jar "/Users/username/Library/Application Support/JetBrains/IntelliJIdea2021.3/plugins/Scala/launcher/sbt-launch.jar" early(addPluginSbtFile="""/private/var/folders/hh/q4qgfsw13_30pqv_tsbd9m_c0000gn/T/idea.sbt""") "; set ideaPort in Global := 49201 ; idea-shell"

I can get rid of a lot of the idea related arguments to run it directly on the command line This is IntelliJ IDEA 2021.3.3 (Ultimate Edition)
I'm using cmake version 3.21.1
The computer is a macbook pro m2 running Ventura/macOS 13.2.1
I installed the command line development tools, not Xcode.

berkeleybarry · 2023-02-23T04:30:56Z

I'm trying to figure out why the CI tests are failing.

The simple project dies on my laptop becuse there is no CMake.txt. The plugin does not know what buildtool to use.

On the CI runs the simple project on dies on Ubuntu with the error below:
CMake Error at /usr/local/share/cmake-3.25/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
[info] [error] Could NOT find JNI (missing: JAVA_INCLUDE_PATH JAVA_INCLUDE_PATH2 AWT)

This looks like the JAVA_HOME is pointing to the wrong place. I can believe the change I made that makes projects with a CMake.txt file work on my laptop causes problems on the Ubuntu build machine. I have no visbility into what is hapening on ubuntu so it is kind of difficult to find something that works on both.

berkeleybarry · 2023-02-23T04:32:47Z

Last comment i garbage. It was unsent draft and I hit some wrong buttons

pomadchin · 2023-02-23T04:41:37Z

@berkeleybarry I think indeed xcode-select --install with a follup terminal restart and targets cleanup should fix all of your issues!

pomadchin · 2023-02-23T04:43:40Z

plugin/src/sbt-test/sbt-jni/overloads/native/src/library.c

+JNIEXPORT jint JNICALL Java_simple_Library_00024_say___3Ljava_lang_String_2
+(JNIEnv *env, jobject clazz, jobjectArray messages) {


@berkeleybarry the array overload!

pomadchin

Thanks for a very nice fix! 🎉

Do you want to incorporate anything else into this PR?

I'm happy to merge it as is and cut a release, and anything else we can do in the follow up PRs.
I'll merge it tomorrow, so you have some time to review changes / add if anything needed.

berkeleybarry · 2023-02-23T06:32:40Z

I'm ok with merging it.

fix names of overloaded methods with ref type parameters

7f2cc78

pomadchin requested changes Feb 22, 2023

View reviewed changes

address PR comments

dd40ed1

pomadchin added the bug label Feb 23, 2023

pomadchin changed the title ~~fix names of overloaded methods with ref type parameters~~ Fix names of overloaded methods with ref type parameters Feb 23, 2023

pomadchin force-pushed the overload_name_fix branch from b04908a to 781aca4 Compare February 23, 2023 03:48

pomadchin reviewed Feb 23, 2023

View reviewed changes

pomadchin force-pushed the overload_name_fix branch from 781aca4 to c619828 Compare February 23, 2023 03:54

Adjust sbt-tests, add an overload test

00605c1

pomadchin force-pushed the overload_name_fix branch from c619828 to 00605c1 Compare February 23, 2023 03:56

pomadchin mentioned this pull request Feb 23, 2023

Expose BuildTools parameters #140

Closed

berkeleybarry closed this Feb 23, 2023

berkeleybarry reopened this Feb 23, 2023

pomadchin force-pushed the overload_name_fix branch from 200e849 to ff4987f Compare February 23, 2023 04:42

pomadchin reviewed Feb 23, 2023

View reviewed changes

pomadchin approved these changes Feb 23, 2023

View reviewed changes

Add more overloads

7038a1a

pomadchin force-pushed the overload_name_fix branch from ff4987f to 7038a1a Compare February 23, 2023 05:03

ReleaseStrings in the array overload

60eba62

pomadchin force-pushed the overload_name_fix branch from 84f8261 to 60eba62 Compare February 23, 2023 05:18

pomadchin merged commit 0fc4dd9 into sbt:main Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix names of overloaded methods with ref type parameters #139

Fix names of overloaded methods with ref type parameters #139

berkeleybarry commented Feb 13, 2023

pomadchin commented Feb 22, 2023

berkeleybarry commented Feb 22, 2023

pomadchin left a comment •

edited

Loading

pomadchin Feb 22, 2023

berkeleybarry Feb 22, 2023

pomadchin Feb 22, 2023 •

edited

Loading

berkeleybarry Feb 22, 2023

pomadchin Feb 22, 2023

berkeleybarry Feb 22, 2023

pomadchin Feb 22, 2023

pomadchin Feb 22, 2023

berkeleybarry Feb 22, 2023

pomadchin Feb 22, 2023

berkeleybarry Feb 22, 2023

berkeleybarry commented Feb 22, 2023

pomadchin commented Feb 22, 2023 •

edited

Loading

pomadchin commented Feb 22, 2023

berkeleybarry commented Feb 22, 2023 via email

pomadchin Feb 23, 2023

berkeleybarry commented Feb 23, 2023 via email

pomadchin commented Feb 23, 2023 •

edited

Loading

berkeleybarry commented Feb 23, 2023

berkeleybarry commented Feb 23, 2023

berkeleybarry commented Feb 23, 2023

berkeleybarry commented Feb 23, 2023

pomadchin commented Feb 23, 2023

pomadchin Feb 23, 2023

pomadchin left a comment

berkeleybarry commented Feb 23, 2023

		@@ -0,0 +1 @@
		This directory contains a header file foe the Overloads class that was generated by the javah application of a Java 8 SDK.

		JNIEXPORT jint JNICALL Java_simple_Library_00024_say___3Ljava_lang_String_2
		(JNIEnv *env, jobject clazz, jobjectArray messages) {

Fix names of overloaded methods with ref type parameters #139

Fix names of overloaded methods with ref type parameters #139

Conversation

berkeleybarry commented Feb 13, 2023

pomadchin commented Feb 22, 2023

berkeleybarry commented Feb 22, 2023

pomadchin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomadchin Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkeleybarry commented Feb 22, 2023

pomadchin commented Feb 22, 2023 • edited Loading

pomadchin commented Feb 22, 2023

berkeleybarry commented Feb 22, 2023 via email

Choose a reason for hiding this comment

berkeleybarry commented Feb 23, 2023 via email

pomadchin commented Feb 23, 2023 • edited Loading

berkeleybarry commented Feb 23, 2023

berkeleybarry commented Feb 23, 2023

berkeleybarry commented Feb 23, 2023

berkeleybarry commented Feb 23, 2023

pomadchin commented Feb 23, 2023

Choose a reason for hiding this comment

pomadchin left a comment

Choose a reason for hiding this comment

berkeleybarry commented Feb 23, 2023

pomadchin left a comment •

edited

Loading

pomadchin Feb 22, 2023 •

edited

Loading

pomadchin commented Feb 22, 2023 •

edited

Loading

pomadchin commented Feb 23, 2023 •

edited

Loading