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

Replace anonymous classes with lambdas #6

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

philwebb
Copy link
Contributor

Whilst looking at the code to asses options for #3 I noticed quite a few anonymous classes that could be replaced with lambdas. If Java 6 compatibility isn't an issue, perhaps these changes will be helpful.

Update the codebase to use lambdas or method references instead of
anonymous classes whenever possible.
@wilkinsona
Copy link

I think this may cause some up-to-date checks to break. Unfortunately, lambdas produce non-deterministic class names at runtime that are unstable from run to run. This prevents Gradle from identifying that the actions haven't changed. There's some more information in Gradle's documentation:

For tracking the implementation of tasks, task actions and nested inputs, Gradle uses the class name and an identifier for the classpath which contains the implementation. There are some situations when Gradle is not able to track the implementation precisely:

  • Unknown classloader
    When the classloader which loaded the implementation has not been created by Gradle, the classpath cannot be determined.

  • Java lambda
    Java lambda classes are created at runtime with a non-deterministic classname. Therefore, the class name does not identify the implementation of the lambda and changes between different Gradle runs.

When the implementation of a task, task action or a nested input cannot be tracked precisely, Gradle disables any caching for the task. That means that the task will never be up-to-date or loaded from the build cache.

I'd be really nice to keep some of the other structural changes, though.

@philwebb
Copy link
Contributor Author

Well that's annoying. Would the following change be enough to fix it? Is there a good way to add a test for the up-to-date checks?

@wilkinsona
Copy link

wilkinsona commented Apr 27, 2020

Sorry, @philwebb. Looking more closely, I think your original changes may be fine. The problem only applies to Actions that are part of a task's execution. Those that are involved only in its configuration should be fine.

Is there a good way to add a test for the up-to-date checks?

Using TestKit and invoking a task twice, checking that the outcome of the first invocation is success and of the second invocation is up-to-date. Here's one example of a test in Spring Boot's Gradle plugin that does this.

@rwinch
Copy link
Contributor

rwinch commented Apr 27, 2020

Thanks for the PR @philwebb. I was under the same impression that lambdas did not work with up to date checking. In hindsight, I think this is only applicable when the lambda is an input or output to the task though.

Perhaps you could switch this back to lambda's and add a test that verifies that rerunning in a new folder, the test is up to date.

@philwebb
Copy link
Contributor Author

I'll revert the second commit and add a test

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants