next.jdbc.prepare depends on next.jdbc.result-set/datafiable-result-set dynamically

[seancorfield]

This prevents GraalVM native compilation of n.j.p and is kind of nasty regardless since it's a circular dependency between the two namespaces.

Can we break that dependency without breaking backward compatibility?

[seancorfield]

This is really because execute-batch! is sort of in the wrong place (it needs to know how to build result sets).

The result-set ns depends on prepare only for create, but that in turn depends on set-parameters which, again, depends on stuff in prepare.

[seancorfield]

I'm in this mess because I didn't keep all the protocols separate from their implementation...

[seancorfield]

Moving execute-batch! to result-set would solve the problem (but break code). Making prepare/execute-batch! not depend on datafiable-result-set would also solve the problem but I'm not sure how to do that without breaking code.

[seancorfield]

@borkdude said "I sometimes work around circular dependencies by creating a volatile in another namespace and vreset-ing the defined function to that volatile, so I can use it from other places. A bit hacky, but it works"

next.jdbc and next.jdbc.result-set (and maybe next.jdbc.sql) could all do that, I guess, to allow next.jdbc.prepare to refer to datafiable-result-set via that volatile...?

[borkdude]

Come to think of it, an alter-var-rootto some uninitialized var will have the same effect and won't require an explicit deref on usage. I wonder why I didn't think of this before.

[seancorfield]

And alter-var-root still works with GraalVM and native code?

[borkdude]

@seancorfield That's a good question. With GraalVM projects I usually use direct linking. So the alter-var-root should happen before any other namespace references that var, else it will be directly linked to nil or unbound, which is a bit brittle. A volatile is a safe way to work around this but marking the "alter-var-rooted" var with :reload should have the same effect. I will try this in one of my own repos.

[borkdude]

@seancorfield I now remember why I went with volatiles.

• Protect against direct linking (alternative is :reload metadata, but this is not applicable to CLJS)
• Compatibility with CLJS. (the same trick works in other environments like the browser, although in CLJS you could probably use set! on the var, which is a bit weird and maybe undocumented)

So all in all that may be the most robust approach for avoiding cyclic dependency problems.

[seancorfield]

@borkdude Could you do me a favor and check whether next.jdbc.prepare will compile and go through the Graal/native process now that I'm using your volatile! hack (HEAD on develop branch) to remove the requiring-resolve call? Was it @holyjak who ran into that as well?

The camel-snake-kebab stuff still does requiring-resolve but only if the CSK library is on the path at compile time since it uses feature macros. If that still causes Graal/native issues, I can take another look at how that is done.

[borkdude]

@seancorfield The GraalVM projects in which I use next-jdbc are still lein/mvn based. So I'm going to try to install this lib using mvn install and hope that that is the correct way.

[borkdude]

Hmm, that doesn't include the sources:

$ ls_jar.clj /Users/borkdude/.m2/repository/seancorfield/next.jdbc/1.1.613/next.jdbc-1.1.613.jar
META-INF/
META-INF/MANIFEST.MF
META-INF/maven/
META-INF/maven/seancorfield/
META-INF/maven/seancorfield/next.jdbc/
META-INF/maven/seancorfield/next.jdbc/pom.xml
META-INF/maven/seancorfield/next.jdbc/pom.properties

This didn't work for me either:

clojure -X:deps mvn-install

[borkdude]

@seancorfield I just copied the sources to the project to be able to test this. Added next.jdbc/prepare and this seems to work fine now without GraalVM compilation blowing up.

I did need to add some reflection config, maybe related to the reflective usage in clojure.data.java for setting properties, but this is manageable.

@holyjak: I tested this with GraalVM 21.0.0 since 20.3.0 hit the MethodHandle issue (for which I also have a fix in 20.3.0, but this isn't needed anymore in 21.0.0). I also needed to a add reflection config. For postgres it now looks like:

[{"name": "java.lang.reflect.AccessibleObject",
  "methods" : [{"name":"canAccess"}]},
 {"name": "org.postgresql.jdbc.PgConnection",
  "methods" : [{"name":"close"}]}]

I published this to a temporary branch called next-jdbc-prepare.

[seancorfield]

Thanks! That confirms what I was trying to fix. I'm most likely going to copy execute-batch! to next.jdbc and deprecate the version in the prepare ns (but leave it there so I don't break anyone's code).

As for the local install, you built a JAR with depstar first?

[borkdude]

@seancorfield What are the exact steps I have to do for it to install into my local ~/.m2?

[seancorfield]

In the next.jdbc repo: clojure -X:jar and then clojure -X:deps mvn-install should work.

I'm a bit surprised you get that weird "empty" JAR in the repo by running mvn-install without a local JAR to copy...