Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

Compile-time compatibility with Java17 #412

Closed
wants to merge 6 commits into from
Closed

Compile-time compatibility with Java17 #412

wants to merge 6 commits into from

Conversation

elefeint
Copy link
Contributor

@elefeint elefeint commented Apr 13, 2022

Part of the way there:

  • Upgrade Gradle to a version compatible with Java 17
  • Replace removed tasks with non-deprecated counterparts.

(fixed with an "I have no idea what I am doing" dog face expression) problem is that integTestCompile.extendsFrom testImplementation seems to behave differently from the old testCompile, so the classpath for integTest task is missing all the source code and imported libraries.

Next problem: a dozen tests fail with a similar test setup problem. Something must have changed in test setup facilities between versions, too.

Caused by: java.lang.IllegalStateException: The settings are not yet available for build 'test'.
	at org.gradle.invocation.DefaultGradle.getSettings(DefaultGradle.java:209)
	at org.gradle.invocation.DefaultGradle_Decorated.getSettings(Unknown Source)
	at org.gradle.buildinit.plugins.BuildInitPlugin.lambda$apply$0(BuildInitPlugin.java:50)
	at org.gradle.api.internal.DefaultMutationGuard$2.execute(DefaultMutationGuard.java:44)
	at org.gradle.api.internal.DefaultMutationGuard$2.execute(DefaultMutationGuard.java:44)
	at org.gradle.configuration.internal.DefaultUserCodeApplicationContext$CurrentApplication$1.execute(DefaultUserCodeApplicationContext.java:123)
	at org.gradle.api.internal.DefaultCollectionCallbackActionDecorator$BuildOperationEmittingAction$1.run(DefaultCollectionCallbackActionDecorator.java:110)
	at org.gradle.internal.operations.DefaultBuildOperationRunner$1.execute(DefaultBuildOperationRunner.java:29)
	at org.gradle.internal.operations.DefaultBuildOperationRunner$1.execute(DefaultBuildOperationRunner.java:26)
	at org.gradle.internal.operations.DefaultBuildOperationRunner$2.execute(DefaultBuildOperationRunner.java:66)
	at org.gradle.internal.operations.DefaultBuildOperationRunner$2.execute(DefaultBuildOperationRunner.java:59)
	at org.gradle.internal.operations.DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:157)
	at org.gradle.internal.operations.DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:59)
	at org.gradle.internal.operations.DefaultBuildOperationRunner.run(DefaultBuildOperationRunner.java:47)
	at org.gradle.internal.operations.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:68)
	at org.gradle.api.internal.DefaultCollectionCallbackActionDecorator$BuildOperationEmittingAction.execute(DefaultCollectionCallbackActionDecorator.java:107)
	at org.gradle.internal.ImmutableActionSet$SetWithManyActions.execute(ImmutableActionSet.java:329)
	at org.gradle.api.internal.DefaultDomainObjectCollection.doAdd(DefaultDomainObjectCollection.java:260)
	at org.gradle.api.internal.DefaultNamedDomainObjectCollection.doAdd(DefaultNamedDomainObjectCollection.java:113)
	at org.gradle.api.internal.DefaultDomainObjectCollection.add(DefaultDomainObjectCollection.java:254)
	at org.gradle.api.internal.DefaultNamedDomainObjectCollection$AbstractDomainObjectCreatingProvider.tryCreate(DefaultNamedDomainObjectCollection.java:944)

@elefeint
Copy link
Contributor Author

The latest issue seems to be an actual problem with Gradle testing facilities. So compile-time Java 17 compatibility is blocked on gradle/gradle#20301

Nothing prevents using app-gradle-plugin with Java 17 at runtime once we pull in the latest core and release.

@elefeint elefeint changed the title Java17 Compile-time compatibility with Java17 Apr 13, 2022
@ludoch
Copy link

ludoch commented Jun 2, 2022

Can we try again? Thanks!

@elefeint
Copy link
Contributor Author

elefeint commented Jun 2, 2022

I'll rebase, and rerun checks, but the underlying issue has not been resolved. We might have to restructure the tests to not rely on this gradle testing functionality.

@elefeint
Copy link
Contributor Author

elefeint commented Jun 2, 2022

I got rid of an extra set of issues by upgrading jacoco, but as far as unit test failures, the same issue remains. It's listed as "known regression" in gradle.

@TWiStErRob
Copy link
Contributor

TWiStErRob commented Jan 3, 2023

This can be unblocked without fixing the upstream issue. That Settings problem is the symptom of eager task configuration and non-idiomatic (old-style) Gradle code.

See the migration path here:
GoogleCloudPlatform/appengine-plugins#999.

Focusing on unblocking this issue alone:

  • I ran the tests locally and found 15 failing tests. I picked one at random: testDetectStandard_withProjectBuilder
    image

  • As you can see here AppEngineCorePluginConfiguration.java:148 is iterating through p.getTasks(). This causes the test to materialize every single task in the build, even though the test only cares about what plugins were applied. And the production code only cares about appengine* tasks.

  • (fix 1) Following lazy task configuration practices, we can look at only the names to pre-filter! tasks before materializing them, so that we don't need the Task object just to get its name:

                      p.getTasks()
                          .getNames()
                          .stream()
                          .filter(taskName -> taskName.startsWith("appengine"))
                          .forEach(taskName ->
                              p.getTasks()
                               .named(taskName)
                               .configure(task -> task.dependsOn(downloadCloudSdkTask))
                          );

    Note that the above code goes one step further, and only sets the dependency if the "taskName" task will be executed (configure only executes when task is created). This makes Gradle configuration faster when no appengine task is executed. For example gradlew jar, gradlew checkstyle, gradlew test, etc. See Gradle docs.

  • At this point I re-ran the tests and started getting a different issue:
    image

  • (fix 2) The problem at AppEngineAppYamlPlugin.java:93 is using some old Gradle/Groovy magic to get the task without actually configuring it full explanation here. There's a specific API for this, called p.getTasks().findByName(), however in these cases we know exactly that these tasks must exist (because of plugin checks) and therefore we can eagerly force-create them:
    image

  • Rerunning the tests once more yield only 2 failures at this point, both of them can be fixed by (fix 1) applied to the corresponding test code.

  • At this point we have a green build, BUT there's more to do

    • There's an instance of .matching( in AppEngineCorePluginConfiguration.createCheckCloudSdkTask which is apparently not covered by unit tests
    • in AppEngineAppYamlPluginTest there are two usages of code similar to (fix 2), search p.getProperties().get.

As a recommendation both of these fixes can be applied independently of this Gradle upgrade (PR title misleading btw).

  • (fix 1) should be safe to apply alone, and would move towards lazy Gradle support as described in Support lazy property evaluation (appengine.stage.setArtifact() doesn't carry task dependencies) appengine-plugins#999. BUT I wouldn't actually recommend fixing it this way. Relying on naming conventions to get tasks is bad practice. Any user could create a custom task (e.g. appengineValidate which might check some ocnfiguration that's actually unrelated to the SDK) and the dependency would be applied to that too unnecessarily. The tasks should be either listed explicitly like in AppEngineStandardPlugin.createStageTask, or rather each task be configured at creation to depend on downloadCloudSdk explicitly. Note: tasks can be turned off after configuration, so you can depend on it always, but turn it off when "invalid" with: downloadCloudSdkTask.onlyIf { managedCloudSdk != null && !p.getGradle().getStartParameter().isOffline() }.
  • (fix 2) is quite safe and would fix Cannot use plugin tasks due to "org.gradle.api.tasks.bundling.Jar.getArchivePath()" because "jar" is null #432 rather than working around it by having the users eagerly create the task / configure the project to avoid this code path.

@ludoch
Copy link

ludoch commented Mar 10, 2023 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants