Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration property to to select the gradle worker isolation strategy #205

Closed
wants to merge 5 commits into from

Conversation

simonhauck
Copy link
Collaborator

@simonhauck simonhauck commented Oct 28, 2023

🚀 Description

As discussed in this , add a new property to select the isolation strategy for the gradle worker.

📄 Motivation and Context

Fixes issue

The user is able to select between the default no-isolation strategy and a process-isolation strategy. I remove the classloader-isolation strategy since it does not seem wo work for larger projects, and I wanted to prevent future issues ;)
But it could be easily added, if desired.

🧪 How Has This Been Tested?

Added integration tests for the format and checkFormat task.
The property is set in the example project.

📦 Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

✅ Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@simonhauck
Copy link
Collaborator Author

simonhauck commented Oct 28, 2023

Hi @cortinico ,

Here is my draft PR. Unfortunately I encountered an issue, that I am not sure how to resolve. Maybe you have an idea?

What happened:
I created the PR, all tests are locally passing. Afterwards I applied the config property in the example project for some final testing.

Isolation strategy parallel works as expected.

No Isolation strategy does not work with the error Unable to load class 'com.facebook.ktfmt.format.ParseError'..
The dependency is not resolved and after some investigation this makes totally sense ;)

In the build.gradle.kts for the plugin we have the following snippet:

dependencies {
    compileOnly(libs.ktfmt)

    // Other dependencies...

    testImplementation(libs.ktfmt)
}

So for the tests, the ktfmt plugin is added as implementation dependency and both strategies work since the dependency is always resolved. But obviously not for the production code. When using the no-isolation strategy, the production classpath is used and there the dependency is only compileOnly and therefore missing.

Ideally we could add the dependency after the configuration phase of the plugin to the classpath, when the isolation strategy is no-isolation. I tried to implement this with org.gradle.api.artifacts.DependencyResolutionListener but was not able to make it work.

The only two other solutions I am able to come up with are:

  • Remove the option again and use either process or no isolation. But options will not make everyone happy as discussed in this issue
  • Or: Always add the dependency as implementation. Users that have a classpath conflict can exclude the dependency as shown in the accepted answer here. I have not yet tested, if that would actually work.

@simonhauck
Copy link
Collaborator Author

For now: Maybe it ist worth to have a quick fix with just the process isolation, so that the plugin starts working again.

And then we have more time to fix it and think about the solution.

import com.ncorti.ktfmt.gradle.GradleWorkerIsolationStrategy

ktfmt {
gradleWorkerIsolationStrategy.set(GradleWorkerIsolationStrategy.NO_ISOLATION)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an example for PROCESS_ISOLATION also here?

@cortinico
Copy link
Owner

No Isolation strategy does not work with the error Unable to load class 'com.facebook.ktfmt.format.ParseError'.. The dependency is not resolved and after some investigation this makes totally sense ;)

Another option could be to ask users to add a classpath() dependency if they wish to use the NO_ISOLATION

  • Or: Always add the dependency as implementation. Users that have a classpath conflict can exclude the dependency as shown in the accepted answer here. I have not yet tested, if that would actually work.

That's also another option. I'm not sure how that would work with the plugin{} syntax though.

If you send another PR with the Process Isolation, we can merge it first, do a new release, and get back to this.

@simonhauck
Copy link
Collaborator Author

Another option could be to ask users to add a classpath() dependency if they wish to use the NO_ISOLATION

That would be in my opinion the better solution. I did not think of that.

If you send another PR with the Process Isolation, we can merge it first, do a new release, and get back to this.

I will do that. Lets first fix it, then make it better :)

@simonhauck
Copy link
Collaborator Author

I would close this PR for now.
When the new version is released, I will do some performance testing with the new process isolation. I still have the hope, that the performance impact is negligible when the gradlew workers are reused between tasks.

Then we still can think of a better solution if necessary :)

@simonhauck simonhauck closed this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatting can fail depending on the number of input files
2 participants