Get feedback from LinkedIn about the new plugin

ymwang1984 commented 5 years ago

Hey, can LinkedIn leave comments in this ticket? Or should LinkedIn create another issue?

big-guy commented 5 years ago

I think feedback here would be good @ymwang1984

ymwang1984 commented 5 years ago

Thanks @big-guy

Unfortunately, the PoC from Play Team at the company left the team recently. So we are trying to sort out the replacement for now. Will have some update about it next week.

Right now, we are trying to enable Gradle 5 for Play apps. There are some sample LinkedIn apps that can run Gradle 5 now (not many, though). Within the mass Gradle 5 migration in the company, we are now prioritizing Play applications.

pioterj commented 5 years ago

@ymwang1984 The general feedback here would be great. Feel free to also open specific issues in this repo.

ymwang1984 commented 5 years ago

Thanks @pioterj

I have not tried the plugin on a sample app. Here are two questions I have in mind after reading the doc only (I could be very wrong though...):

(1) After the software model is gone (which is good for the long term), where will the existing setting go? Such as "custom twirl templates".

model { 
  components { 
    play { 
      sources { 
        withType(TwirlSourceSet) { 
          // use template format StreamFormat for all files named *.scala.stream 
          // additionally, include com.linkedin.sbt.streaming._, etc. imports in generated sources 
          addUserTemplateFormat('stream', 'com.linkedin.sbt.streaming.StreamFormat', 'com.linkedin.sbt.streaming._', 'com.linkedin.playplugins.streaming.StreamUtils._') 
        } 
      } 
    } 
  } 
}

(2) Is the hot reload mechanism changed? Do we need to add "--continuous" to have it now?

pioterj commented 5 years ago

@bmuschko Could you answer the questions above?

bmuschko commented 5 years ago

@ymwang1984 Find my answers below.

(1) After the software model is gone (which is good for the long term), where will the existing setting go? Such as "custom twirl templates".

In the "old" plugin this is only documented in the Javadocs of the SourceSet and not the user guide. Looks like we don't have the same Javadocs documentation for this plugin. I'll create an issue for it. The new SourceSet implementation is org.gradle.playframework.sourcesets.TwirlSourceSet. Basically you do it as such:

sourceSets {
    main {
        twirl {
            userTemplateFormats.add(newUserTemplateFormat("unused", "views.formats.unused.UnusedFormat"))
        }
    }
}

There's also a test case that demonstrates the usage.

(2) Is the hot reload mechanism changed? Do we need to add "--continuous" to have it now?

Continuous build is a cross-cutting concern. It will work the same as with the software model. You just add --continuous.

ymwang1984 commented 5 years ago

Thank you very much @bmuschko for the clear explanation.

Do we have some sample apps in the repo to test it? I recall the sample apps in Gradle repo was quite helpful in learning, debugging and verification.

bmuschko commented 5 years ago

@ymwang1984 You can find the samples here: https://github.com/gradle/playframework/tree/master/src/docs/samples. You can also find the link in the README.adoc.

pioterj commented 5 years ago

@ymwang1984 Note that you need to add the plugin version manually to make the samples work. See https://github.com/gradle/playframework/issues/76.

ymwang1984 commented 5 years ago

@pioterj and @bmuschko

Thank you! Very helpful information.

I tried one or two samples today briefly, I saw bunch of Test compilation failure in "compileTestScala". Looks like the dependency for Specs2 or Play test fixtures are missing in the samples. Could you confirm? I can do some hot reloads, though.

bmuschko commented 5 years ago

@ymwang1984 See https://github.com/gradle/playframework/issues/74. I believe they also fail in Gradle core right now. It's something we detected along the way.

ymwang1984 commented 5 years ago

Thanks @bmuschko so it is expected.

Is there ETA to fix that? Adding specs2 dependency still does not work for me locally.

It is not blocking since I will still be able to verify some other stuff. Thanks.

bmuschko commented 5 years ago

@ymwang1984 No ETA at the moment but I think soon. We were trying to gather as much feedback as possible to amount a big enough work package to keep a developer focused on the work for a couple of days at once. The more input you can give us the better.

ymwang1984 commented 5 years ago

Is this issue fixed? https://github.com/gradle/gradle/issues/6789

We also had a support request ticket: https://support.gradle.com/hc/en-us/requests/1937

bmuschko commented 5 years ago

@ymwang1984 I think it's best to follow up on this in the support ticket.

ymwang1984 commented 5 years ago

Ok I will follow up with @big-guy on this.

bmuschko commented 5 years ago

@ymwang1984 We fixed the sample code (failing test) you were asking about before and added more Javadocs documentation. Did you have a chance to test the plugin with some other projects in the meantime?

ymwang1984 commented 5 years ago

@bmuschko

Thanks for the fix! I will try to verify some time later this week.

We are working on migrating Play applications to Gradle 5 at the moment, including the largest one (our Flagship product). They hit some issues and we are trying to resolve.

I am still curious about the solution of this bug fix request: https://github.com/gradle/gradle/issues/6789

It's still not entirely clear where the fix would go. Should it go to the new plugin? cc @big-guy

For LinkedIn, the issue is not really about the build time display. Multiple teams observed this behavior and would like to see the server up and running "the first time it is loaded". For a very large application, even if all steps are UP-TO-DATE the second round, it still adds quite some time to the development cycle. I'm happy to discuss with more details.

@garylin (manager of Play team at LinkedIn) may also want to open an issue about Gradle unable to support "-Dhttp.port=disabled"

Context:

https://www.playframework.com/documentation/2.6.x/ConfiguringHttps#Turning-HTTP-off https://stackoverflow.com/questions/24864163/how-to-disable-http-port-in-play-framework

ymwang1984 commented 5 years ago

@bmuschko

Today one team hit the annotation processor problem when they attempted to onboard Gradle 5. In the end, we have a workaround. The issue was documented in

https://github.com/gradle/gradle/issues/8378

The issue for this plugin is documented in

https://github.com/gradle/playframework/issues/84

bmuschko commented 5 years ago

@ymwang1984 You should be able to work around the issue in https://github.com/gradle/playframework/issues/84 in a similar fashion. See my comment.

bmuschko commented 5 years ago

@ymwang1984 Just letting you know that https://github.com/gradle/playframework/issues/84 has been fixed and a new version has been released. Did you have a chance to try out this plugin with one of your project?

ymwang1984 commented 5 years ago

@bmuschko Great news!

Currently we are still working on Gradle 5 upgrade for Play projects. Will update you once we have projects that try it with Lombok.

So I assume (with Gradle core) we will still use the workaround mentioned in https://github.com/gradle/gradle/issues/8378 ?

bmuschko commented 5 years ago

@ymwang1984

Currently we are still working on Gradle 5 upgrade for Play projects. Will update you once we have projects that try it with Lombok.

OK, thanks.

So I assume (with Gradle core) we will still use the workaround mentioned in gradle/gradle#8378 ?

That's correct.

ymwang1984 commented 5 years ago

@bmuschko

Sorry for the late reply since our team is still working on Gradle 5 upgrade. But good news is that now we've got sufficient number of applications using Gradle 5.

We are testing it out with some of the apps. And we are hit with this slf4j binding problem: https://github.com/gradle/playframework/issues/90

Would appreciate if you could shed some light about how to work around this, thank you!

also cc @big-guy @pioterj

aaronlijie commented 5 years ago

This is Jie, working with @ymwang1984. @bmuschko The following is the stacktrace for our issue. It is pretty similiar with #90

SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/Users/jieli/.gradle/caches/modules-2/files-2.1/com.linkedin.spark-deployer-java/spark-deployer-java-impl/0.0.8/48c281be730e593d7cdfb74922162e37c37fedf5/spark-deployer-java-impl-0.0.8.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/Users/jieli/.gradle/ligradle/gradle-5.2.1/lib/gradle-logging-5.2.1.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [ch.qos.logback.classic.util.ContextSelectorStaticBinder]
java.lang.ExceptionInInitializerError
        at java.io.ObjectStreamClass.hasStaticInitializer(Native Method)
        at java.io.ObjectStreamClass.computeDefaultSUID(ObjectStreamClass.java:1787)
        at java.io.ObjectStreamClass.access$100(ObjectStreamClass.java:72)
        at java.io.ObjectStreamClass$1.run(ObjectStreamClass.java:253)
        at java.io.ObjectStreamClass$1.run(ObjectStreamClass.java:251)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.io.ObjectStreamClass.getSerialVersionUID(ObjectStreamClass.java:250)
        at java.io.ObjectStreamClass.initNonProxy(ObjectStreamClass.java:611)
        at java.io.ObjectInputStream.readNonProxyDesc(ObjectInputStream.java:1623)
        at java.io.ObjectInputStream.readClassDesc(ObjectInputStream.java:1518)
        at java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:1774)
        at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1351)
        at java.io.ObjectInputStream.defaultReadFields(ObjectInputStream.java:2000)
        at java.io.ObjectInputStream.readSerialData(ObjectInputStream.java:1924)
        at java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:1801)
        at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1351)
        at java.io.ObjectInputStream.readObject(ObjectInputStream.java:371)
        at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:112)
        at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:68)
        at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:62)
        at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:67)
Caused by: java.lang.ClassCastException: ch.qos.logback.classic.Logger cannot be cast to org.gradle.api.logging.Logger
        at org.gradle.api.logging.Logging.getLogger(Logging.java:38)
        at org.gradle.playframework.tools.internal.run.PlayWorkerServer.<clinit>(PlayWorkerServer.java:22)
        ... 21 more

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':pemberly-skeleton-api-frontend:runPlay'.
> Process 'Gradle Play Worker 19' finished with non-zero exit value 1

bmuschko commented 5 years ago

@aaronlijie @ymwang1984 Thanks for reporting. Could you please put together a very simple project that reproduces the issue as requested here https://github.com/gradle/playframework/issues/90#issuecomment-478677674? You likely have two different implementations of SLF4J on the classpath. My guess is that the worker API bleeds Gradle's dependencies into the classpath of the plugin. You can probably work around it by excluding the transitive SLF4J dependency.

aaronlijie commented 5 years ago

Hi that spark-deployer-java-impl-0.0.8.jar is a fat jar which has org.slf4j.impl.xxx. I have excluded spark-deployer-java-impl and logback. I confirmed that they were not in the build scan/ Dependencies. However, when I execute runPlay， The issue still existed:

SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/Users/jieli/.gradle/caches/modules-2/files-2.1/com.linkedin.spark-deployer-java/spark-deployer-java-impl/0.0.8/48c281be730e593d7cdfb74922162e37c37fedf5/spark-deployer-java-impl-0.0.8.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/Users/jieli/.gradle/ligradle/gradle-5.2.1/lib/gradle-logging-5.2.1.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [ch.qos.logback.classic.util.ContextSelectorStaticBinder]
java.lang.ExceptionInInitializerError

bmuschko commented 5 years ago

@aaronlijie Optimally, we'd trim down your example to minimal dependency that produces the issue. I have not seen the issue one some of the sample projects. Any chance you can put something together.

@big-guy Any advice you can give from the Gradle core side of things?

aaronlijie commented 5 years ago

I tried to reproduce it (add dependencies) in the sample projects, but I couldn't. And our project is coupled with linkedin's settings a lot and not easy to decouple. Is it possible to fake classloader to let SLF4J bind to ch.qos.logback.classic.util.ContextSelectorStaticBinder?

Update: Let me updated something from my side. spark-deployer-java-impl-0.0.8.jar is a fat jar which has org.slf4j.impl.xxx. I totally removed it from dependency and finally made the runPlay work. In the ticket #90,I think that swagger-codegen-cli is also a fat jar. Hence I guess the issue is, if we have a fat jar which contains slf4j stuff, and that fat jar is loaded before other SLF4J, it will cause the issue.

aaronlijie commented 5 years ago

Here is a new issue: We have another plugin which will do the following:

Set<File> sourceDirs = project.ideaModule.module.sourceDirs
sourceDirs.addAll(sourceSet.java.srcDirs)

It worked in the past and didn't work well with the new play gradle plugin. The stacktrace is

Caused by: org.gradle.api.internal.plugins.PluginApplicationException: Failed to apply plugin [id 'li-pegasus2']Open stacktrace
Caused by: java.lang.UnsupportedOperationException: (No message provided)Close stacktrace
at java_util_Set$addAll.call(Unknown Source)
at java_util_Set$addAll.call(Unknown Source)
at com.linkedin.pegasus.gradle.PegasusPlugin.addGeneratedDir(PegasusPlugin.groovy:868)
at com.linkedin.pegasus.gradle.PegasusPlugin.configureDataTemplateGeneration(PegasusPlugin.groovy:1357)
at org.gradle.internal.metaobject.BeanDynamicObject$MetaClassAdapter.invokeMethod(BeanDynamicObject.java:479)

I think the issue is related to: https://github.com/gradle/playframework/blob/95e68fe1a90dc40b9ce8b704e1c902369a67490c/src/main/java/org/gradle/playframework/plugins/PlayIdeaPlugin.java#L51

In the new plugin, it returns a unmodifiableSet. In the past I think the set is modifiable.

bmuschko commented 5 years ago

@aaronlijie I opened a pull request for your issue with the IDEA plugin: https://github.com/gradle/playframework/pull/91. Could you please create completely new GitHub issues if you encounter a problem and then link to them from here?

I will have to see if I can somehow reproduce your issue with SLF4J.

bmuschko commented 5 years ago

@aaronlijie Regarding the SLF4J issue I added a longer comment to https://github.com/gradle/playframework/issues/90 which applies to you as well. Sounds like you could work around it in the meantime.

bmuschko commented 5 years ago

@aaronlijie @ymwang1984 A fix for https://github.com/gradle/playframework/pull/91 has been release with version 0.5 of the plugin. Could you please give it a try?

mkurz commented 5 years ago

@bmuschko Can you push the v0.5 tag? https://github.com/gradle/playframework/releases

bmuschko commented 5 years ago

@mkurz done

aaronlijie commented 5 years ago

@aaronlijie @ymwang1984 A fix for #91 has been release with version 0.5 of the plugin. Could you please give it a try?

I have tried and the issue is fixed.

bmuschko commented 5 years ago

@aaronlijie @ymwang1984 Great, can you let us know as soon as you successfully adopted this plugin?

aaronlijie commented 5 years ago

@bmuschko Hi I created an separate #95 about " createXxxxxDistributionJar task behaves different under new playframework"

bmuschko commented 5 years ago

@aaronlijie A fix for https://github.com/gradle/playframework/issues/95 has been released with version 0.6 of the plugin.

bmuschko commented 5 years ago

@aaronlijie @ymwang1984 Is there anything left that needs to be addressed from our end until you can make a full transition to the new plugin?

aaronlijie commented 5 years ago

Hi @bmuschko I create a issue about behavior change under new playFramework: #101.

Here is update from our side: we almost make our first project work with new playFramework, we plan to switch to new playFramework for that project in the following few days.

aaronlijie commented 5 years ago

Hi @bmuschko I create a ticket about compileScala task. #109

It seems compilePlayRoutes task will make compileScala task always run instead of from_cache under some cases. Would you like to look at it ?

ymwang1984 commented 5 years ago

Hey @big-guy

Thanks for the release and the fix!

We noticed that it probably requires the latest Gradle 5.5. Is it true? Is it related to the latest fix in Worker API, etc?

We'd like the upgrade of this plugin to be decoupled from the upgrade of Gradle, if possible. It (coupling) may make migration more difficult for applications. Hope that makes sense :)

big-guy commented 5 years ago

@ymwang1984 I bumped the plugin build to 5.5.1, but I don't believe there's anything that ties the plugin to 5.5. I've been able to run the build with 5.4.1. I didn't go any older than that, but there have been few changes to the plugin since ~5.2.

aaronlijie commented 5 years ago

Hi @big-guy Thank you for your reply. I tested your release (playframework 0.7) with different gradle version in our project. The following is the result:

5.5.1 and 5.4.1 could work
5.2.1 couldn't work ( 5.2.1 is the version adopted at Linkedin) the following is the error:

What went wrong: A problem occurred evaluating project ':play-skeleton-frontend-on-gradle'.

Failed to apply plugin [class 'org.gradle.playframework.plugins.PlayApplicationPlugin'] Could not create task ':play-skeleton-frontend-on-gradle:runPlay'. Could not create task of type 'PlayRun'. org.gradle.api.model.ObjectFactory.fileCollection()Lorg/gradle/api/file/ConfigurableFileCollection;

I searched and found ObjectFactory.fileCollection() is available sicne 5.3

Hence I guess the following code caused the issues and required the gradle upgrade

big-guy commented 5 years ago

Hmm, if that's the only problem, we can probably make it work with 5.2.1

aaronlijie commented 5 years ago

Hi @big-guy Thank you for your reply. According to the user guide : https://gradle.github.io/playframework/

The build has to use Gradle 5.0 or higher

So I think it is better to not couple with any specific version higher than 5.0.

benmccann commented 4 years ago

I've since left LinkedIn, but thought I'd provide some feedback anyway since I was previously working on this code:

JavaScriptMinify could be deleted or moved to a separate plugin. There's no built-in JavaScript minification in Play as far as I'm aware, so I'm not sure why this is included. In Play people most often use a native JavaScript toolchain like webpack though you could also add an sbt plugin like sbt-uglify, but I don't think Play does anything here by default. LinkedIn doesn't need it as far as I'm aware
I'm not sure why there's a PlayIdeaPlugin. Why can't we use Gradle's core IDEA plugin with the proper configuration? If the Play IDEA plugin is doing something special it'd be nice to add some code comments explaining
The Twirl compiler could be made a separate plugin. Twirl can be used outside of Play. The Play sbt plugin automatically includes the Twirl sbt plugin, which can be disabled. LinkedIn doesn't need Twirl. It would also make the codebase more modular and easier to maintain
The tests take a really long time to run. I'm not sure if there's anyway to speed this up, but I gave up after 30 min. The continuous build took about an hour and a half to complete. Splitting out the above packages might at least move those tests into a separate project making each have a smaller test suite

gradle / playframework

Get feedback from LinkedIn about the new plugin #77