-
Notifications
You must be signed in to change notification settings - Fork 5k
Preliminary Play 2.7 support #6374
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
Conversation
With akka-grpc from the akka/akka-grpc#302 branch and gradle from the gradle/gradle#6374 branch
With akka-grpc from the akka/akka-grpc#302 branch and gradle from the gradle/gradle#6374 branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far. Need to change tests too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add another case without repeting the RoutesCompilerAdapterV24X
creatiion:
case PLAY_2_6_X:
case PLAY_2_7_X:
return new RoutesCompilerAdapterV24X(playVersion, scalaVersion);
subprojects/platform-play/src/main/java/org/gradle/play/internal/platform/PlayMajorVersion.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I made some updates, but the integration tests fail because they rely on |
Hum... Is it possible to isolate the tests for 2.7 to force the use of @blindpirate @marcphilipp WDYT? |
The newest version in the play 2.3 series was released back in 2015... feels like it might be OK to drop support for it? Or do we need to go through a deprecation cycle for that first? |
Removal of 2.3 needs a proper deprecation cycle to be removed with Gradle 6.0. @raboof Could you please post an example of such a problematic test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Play 2.7.0-RC3 is released, so I updated the references to M2
subprojects/platform-play/src/main/java/org/gradle/play/internal/platform/PlayMajorVersion.java
Outdated
Show resolved
Hide resolved
...tform-play/src/integTest/groovy/org/gradle/play/integtest/PlayPlatformIntegrationTest.groovy
Outdated
Show resolved
Hide resolved
...jects/platform-play/src/test/groovy/org/gradle/play/internal/PlayPlatformResolverTest.groovy
Outdated
Show resolved
Hide resolved
...jects/platform-play/src/test/groovy/org/gradle/play/internal/PlayPlatformResolverTest.groovy
Outdated
Show resolved
Hide resolved
...ects/platform-play/src/test/groovy/org/gradle/play/plugins/PlayDistributionPluginTest.groovy
Outdated
Show resolved
Hide resolved
...platform-play/src/testFixtures/groovy/org/gradle/play/integtest/fixtures/PlayCoverage.groovy
Outdated
Show resolved
Hide resolved
...platform-play/src/testFixtures/groovy/org/gradle/play/integtest/fixtures/PlayCoverage.groovy
Outdated
Show resolved
Hide resolved
...rojects/platform-play/src/main/java/org/gradle/play/internal/twirl/TwirlCompilerFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"2.7.0-RC3" | "play.core.server.ProdServerStart" | |
"2.7.0" | "play.core.server.ProdServerStart" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code format got a little weird here.
Testing with 2.7.0-M2 now, not thoroughly tested yet though. Signed-off-by: Arnout Engelen <[email protected]>
Signed-off-by: Arnout Engelen <[email protected]>
Signed-off-by: Arnout Engelen <[email protected]>
Unfortunately this will fail because some tests rely on StaticRoutesGenerator which was deprecated in 2.6.0 and removed in 2.7.0 https://github.com/playframework/playframework/blob/a2186e10178f7084c9493ecc8246b615bf108371/documentation/manual/releases/release27/migration27/Migration27.md#staticroutesgenerator-removed Signed-off-by: Arnout Engelen <[email protected]>
Signed-off-by: Renato Cavalcanti <[email protected]>
Signed-off-by: Renato Cavalcanti <[email protected]>
Signed-off-by: Renato Cavalcanti <[email protected]>
I think this is now complete and good to review/merge. It is not removing Play 2.3 anymore so that it does not need to go through the deprecation cycle. WDYT? |
Please merge. |
@big-guy / @marcphilipp, is there anything missing to get this merged? |
@renatocaval We want to get this in, but we're also trying to extract the Play plugin into a separately releasable plugin: https://github.com/gradle/playframework We're not going to get to this PR for the next release (5.4), but I want to circle back on this for the following release. |
From #6362 (comment) it seems the new plan is to migrate/port, not merge this PR. |
We may merge this as-is but the expectation should be to migrate to gradle/playframework. At the moment, this plugin is simply the extraction of the play plugin from the Gradle core to use the current model instead of the software model. |
@lacasseio - Any update on this item? Whether extracting or merging here, I believe many of us are currently without a path forward. |
@arjunchhabra Let's migrate the change to For everyone wanting this to be part of the core plugin, let's instead focus on identifying the blocker preventing you from migrating to the |
I'm going to close this as the playframework plugin has these changes now and should be released shortly. |
Testing with 2.7.0-M2 now, not thoroughly tested yet though.
Context
Resolve issue #6362
Contributor Checklist
<subproject>/src/integTest
) to verify changes from a user perspective<subproject>/src/test
) to verify logic./gradlew <changed-subproject>:check
Gradle Core Team Checklist