Make SqlDelightWorkerTask more configurable, and update default configuration to support developing on Windows
#5215
Conversation
|
During further tests, I found that process isolation also results in
It can be fixed by passing through JvmArgs, which may result in unexpected consequences, passing predefined heap size, or reverting the process isolation. I've opted for fixed 2GB max heap size. Also, implementing CI tests for this situation to prevent recurrence would be great. |
| // This breaks retrieval of environment variables like temporary directories in the worker | ||
| // process, which are required for JDBC to work correctly. | ||
| // Therefore, we explicitly forward all environment variables to the worker process. | ||
| forkOptions.environment(System.getenv()) |
There was a problem hiding this comment.
Instead forwarding all environment variables we should add a MapProperty to the task and pass the property here. Then you can opt-in passing your specific environment variables using tasks.verifyMigrations/tasks.withType(SqlDelightWorkerTask::class)
There was a problem hiding this comment.
@hfhbd Thank you for your response. I can see that your solution is cleaner and more customizable, yet I have a few concerns.
If we avoided forwarding TEMP environment variables, Any team with developers on Windows machines would have to forward them which adds more code to the minimal setup. Do you think that in addition to adding a property to customize them, we can forward the bare-minimum env variables and increase the maximum heap size by default? Also, the heap size limitation may affect developers on other hosts as well.
Also, to what extent should the tasks be customizable? just environment and maxHeapSize? Or also add systemProperties, jvmArgs, and minHeapSize? Should we add functions like environment(), and jvmArgs()?
SqlDelightWorkerTask more configurable, and update default configuration to support developing on Windows
|
I've made the task more configurable by adding A further step would be to implement I'd appreciate your review since this affects the API a lot. |
This commit replaces `getInheritableEnvironmentVariables()` usage with a copied version of this function. I've searched online to find a cleaner solution but it seems that there is no alternative API intended for public use.
37215fe to
d53c9d1
Compare
|
@hfhbd Would you mind reviewing the current state of the PR? |
|
needs a spotless run |
Thanks for the review. I have applied it. |
Migration to process isolation in #5068 resulted in broken migration verification, and plugin actions on Windows. This is due to JDBC's usage of temporary directory which cannot be retrieved from environment variables as they are, deliberately or by mistake, not passed to isolated process by default.
This PR makes
SqlDelightWorkerTaskmore configurable by exposing properties likeenvironment,maxHeapSize, ...etc. It also sets the "convention" value of these properties so that developing on Windows is no longer broken. It also may fixOut of memory. Java heap spaceexceptions on other machines.Fixes #5129