Bring back ability to disable ClassLoader isolation #3654
Conversation
C2H6O
force-pushed
the
lx/disableClassLoaderIsolation
branch
from
fb48006
to
b003a24
Compare
|
I think we should use classloader isolation only and provide a configuration: https://docs.gradle.org/current/userguide/worker_api.html#changing_the_isolation_mode See also: #3644 |
|
@hfhbd our use case is the following: TokenizingDriver:
|
|
@hfhbd @AlecStrong is merging this PR something that you are willing to entertain? I'm trying to figure out if we should setup a fork of SqlDelight with these changes or not. Without these changes we are unable to run the migration verification tasks with our custom Sqlite driver 🙏 |
|
So the real problem is the autoregistration of the jdbc driver? If we use the classpath and configuration gradle option, then you could simple override this dependency, I guess. I recently tried this approach in another project and it works nicely, but I didn't test it with default dependencies. |
|
do you mind running spotlessApply on this branch? In general I'm not reviewing PRs that don't go green (I check to see if it was a flake since those do happen sometimes) since the tests 9 out of 10 times do a better job reviewing code than i do |
|
I'm not opposed to this, but I'm pretty sure we just cant operate with classloaderIsolation disabled anymore because of how dialects are loaded now. However if all the tests pass, it might be fine |
C2H6O
force-pushed
the
lx/disableClassLoaderIsolation
branch
from
467e7bb
to
f353f53
Compare
C2H6O
force-pushed
the
lx/disableClassLoaderIsolation
branch
from
f353f53
to
1b5cc7f
Compare
|
✅ |
|
Looks like the new test in this PR is failing... I'll have to look into this on Monday, but does that look like a failure that you were expecting @AlecStrong? If yes, then I'm not really sure what options there are for registering our own driver before migrations get run. Do you have any suggestions? |
|
@C2H6O Like I said, I would try to use I use this in a similar project: val kobolFlowGraphPlugin = project.configurations.register("kobolFlowGraphPlugin") {
it.isCanBeResolved = true
it.isCanBeConsumed = false
}
project.tasks.register("flowGraph", KobolFlowGraph::class.java) {
it.classpath.from(kobolFirPlugin, kobolFlowGraphPlugin.map {
it.defaultDependencies {
it.add(project.dependencies.create("app.softwork:kobol-plugins-flow-graph-plantuml:$version"))
}
})
}
public abstract class CreateFlowGraph : WorkAction<CreateFlowGraph.Parameters> {
public interface Parameters : WorkParameters {
public val inputFiles: ConfigurableFileCollection
public val outputFolder: DirectoryProperty
}
override fun execute() {
val inputs: Set<File> = parameters.inputFiles.files
val outputFolder = parameters.outputFolder.get().asFile
val firPlugins = ServiceLoader.load(FirPluginBeforePhase::class.java) + ServiceLoader.load(
FirPluginAfterPhase::class.java
)
for (flowGraph in ServiceLoader.load(FlowGraphFactory::class.java)) {
inputs.toTree(firPlugins + flowGraph(outputFolder))
}
}
}
// Fetched with the ServiceLoader
public fun interface FlowGraphFactory {
public operator fun invoke(outputFolder: File): FlowGraph
}
plugins {
id("app.softwork.kobol")
}
dependencies {
// optional overwrite this dependency or use app.softwork:kobol-plugins-flow-graph-plantuml by default
kobolFlowGraphPlugin(...)
}
tasks.flowGraph {
sources.from("foo.cbl")
}The JDBC Drivers are also fetched with the ServiceLoader mechanism, so I guess it will work. @AlecStrong If we want to use more Gradle configurations to setup the dependencies, I will create a PR (and copy this test :) |
|
@C2H6O How do you setup your Driver at the moment? Could you share your code? |
|
@hfhbd here's all the code relevant to the driver (and the driver itself, just for completeness): In one of the It does 2 things:
The main issue is that the Let me know if I've missed anything and can provide more info! |
|
@hfhbd after spending some time on this, my guess is that I would add this logic into |
|
@C2H6O Yes, this is the idea. |
|
Closing this in favor of #3986 |
This PR brings back functionality that was introduced in #2082 but was recently removed 077837a#diff-202cb8a4767ef0496a1551b59461db6625ca9aad2ddf92fefc467a07887cfe01. We rely on shared ClassLoaders for setting up a Sqlite Driver (with a tokenizer) prior to running the migrations.
I took a slightly different approach from #2082 by introducing the
disableClassLoaderIsolationproperty on theVerifyMigrationTask, instead ofSqlDelightWorkerTask. Not sure which approach is preferred, let me know!Related issue: #3618