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

Make PlatformScalaCompile Cacheable. Issue:#3648 #3804

Merged
merged 6 commits into from Jan 30, 2018

Conversation

Projects
None yet
5 participants
@devishree90
Copy link
Contributor

commented Dec 13, 2017

Context

Issue: #3648
This change will make the tasks like compilePlayBinaryScala as Cacheable. (I'm working on play project which uses gradle as build system. we are trying to levarage Build caching to improve build time. Currently, most of the scala tasks depend on 'compilePlayBinaryScala' and 'compilePlayBinaryScala' not being cacheable triggers cascading non-cacheability on other tasks as well)

Please suggest recommended tests for this change.

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commmits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

@devishree90 devishree90 force-pushed the devishree90:master branch from 89b87f0 to 33673bf Dec 13, 2017

@wolfs

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

Hi @devishree90, thank you for your PR!

Regarding test coverage: I would expect this change to have the same coverage as we do for the ScalaCompile task which is created by the scala plugin. The following tests come to mind:

Since you are especially interested in making compilePlayBinaryScala cacheable, I would suggest to add tests for this scenario. There should be tests to cover the scenarios described in the build cache guide.

@wolfs wolfs added a:feature and removed a:bug labels Dec 19, 2017

@devishree90 devishree90 force-pushed the devishree90:master branch 2 times, most recently from faa3b0b to e66274a Jan 8, 2018

@devishree90

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2018

@wolfs - I have added tests as suggested. I have a failing test which i need help with. Scenario which verifies Caching could be used from two different directories is failing in this commit. I suspect this could be because of the directory structure which i'm creating/using. (I compared tmp/CompilePlayBinaryScala.analysis in both folders and only difference i see is directory path.
). Could you help me investigate this failure?

@eriwen

This comment has been minimized.

Copy link
Member

commented Jan 9, 2018

@wolfs or @lptr can I assign this to you?

devishree90 added some commits Dec 13, 2017

Make PlatformScalaCompile Cacheable. Issue:#3648
Signed-off-by: Devi Sridharan <devishree_90@yahoo.co.in>
Adding tests for PlatformScalaCompile
Issue:#3648

Signed-off-by: Devi Sridharan <devishree_90@yahoo.co.in>

@devishree90 devishree90 force-pushed the devishree90:master branch from 8192843 to caf283d Jan 20, 2018

@devishree90

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2018

@wolfs @lptr @eriwen - Is anyone working on this PR?

@lptr

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

There's been some holidays, but the team is back now. We'll take a look at this PR this week.

@devishree90

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2018

Thank you @lptr

@wolfs wolfs self-assigned this Jan 23, 2018

@wolfs
Copy link
Member

left a comment

Thanks for the update on the PR!
Awesome, now we have some tests!
The tests already show a problem: source in PlatformScalaCompile requires the correct relocatability annotations. Moreover, the Java version should also be tracked when compiling Scala sources.

repositories {
${RepoScriptBlockUtil.jcenterRepositoryDefinition()}
${RepoScriptBlockUtil.lightbendMavenRepositoryDefinition()}
${RepoScriptBlockUtil.lightbendIvyRepositoryDefinition()}

This comment has been minimized.

Copy link
@wolfs

wolfs Jan 23, 2018

Member

Do we need the ivy and maven lightbend repositories here? I think the maven ones should be enough, right?

print("Hello!")
}
}
""".stripIndent()

This comment has been minimized.

Copy link
@wolfs

wolfs Jan 23, 2018

Member

This probably needs to be indented 4 spaces more. I would leave out the .stripIndent(), since it only makes the test a little bit harder to read.
I prefer indenting multiline strings like this:

    def multiLineString = """
        This is the text
        which goes over multiple lines
    """
@@ -29,6 +30,7 @@
/**
* A platform-aware Scala compile task.
*/
@CacheableTask
@Incubating
public class PlatformScalaCompile extends AbstractScalaCompile {

This comment has been minimized.

Copy link
@wolfs

wolfs Jan 23, 2018

Member

As seen by the failing compilation is cached if the build executed from a different directory, it is not enough to only add @CacheableTask to PlatformScalaCompile. It is also necessary to add the override of getSource with the correct path sensitivity:

/**
* {@inheritDoc}
*/
@Override
@PathSensitive(PathSensitivity.NAME_ONLY)
public FileTree getSource() {
return super.getSource();
}

Moreover, the Java version should be tracked for Scala compilation, too. For ScalaCompile, this is done here:

/**
* The Java major version of the JVM the Scala compiler is running on.
*
* @since 4.1
*/
@Incubating
@Input
// We track this as an input since the Scala compiler output may depend on it.
// TODO: This should be replaced by a property in the Scala toolchain as soon as we model these.
protected String getJvmVersion() {
return JavaVersion.current().getMajorVersion();
}

We should add a test for PlatformScalaCompile, too, similar to this:

def "compile is out of date when changing the java version"() {
def jdk7 = AvailableJavaHomes.getJdk(VERSION_1_7)
def jdk8 = AvailableJavaHomes.getJdk(VERSION_1_8)
buildScript(scalaProjectBuildScript('0.3.13', '2.11.8'))
when:
executer.withJavaHome(jdk7.javaHome)
run 'compileScala'
then:
executedAndNotSkipped(':compileScala')
when:
executer.withJavaHome(jdk7.javaHome)
run 'compileScala'
then:
skipped ':compileScala'
when:
executer.withJavaHome(jdk8.javaHome)
run 'compileScala', '--info'
then:
executedAndNotSkipped(':compileScala')
}

I suggest adding both to AbstractScalaCompile.

This comment has been minimized.

Copy link
@devishree90

devishree90 Jan 25, 2018

Author Contributor

Updated as suggested


import static org.gradle.integtests.fixtures.RepoScriptBlockUtil.jcenterRepository

class PlayCompilationFixture {

This comment has been minimized.

Copy link
@wolfs

wolfs Jan 23, 2018

Member

This test fixture doesn't seem to be very idiomatic for play. IOW, I think this should be a somewhat minimal play application with the corresponding classes.
It would probably also be nice to see the caching working under the presence of routes and twirl templates.

This comment has been minimized.

Copy link
@devishree90

devishree90 Jan 25, 2018

Author Contributor

I have added classes for minimal play application. Please let me know if more changes are required

devishree90 added some commits Jan 23, 2018

updating Tests with review comments
Signed-off-by: Devi Sridharan <devishree_90@yahoo.co.in>

@devishree90 devishree90 force-pushed the devishree90:master branch from 99af45a to 3b7adcd Jan 25, 2018

@wolfs

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

The CI build currently fails: https://builds.gradle.org/viewLog.html?buildId=10556883&tab=buildResultsDiv&buildTypeId=Gradle_Check_SanityCheck
I guess this line needs to be added to language-scala.gradle:

useTestFixtures(project: ":plugins") // includes core test fixtures

devishree90 added some commits Jan 25, 2018

Added Test to verify Java version updates with Cache & update languag…
…e-scala.gradle

Signed-off-by: Devi Sridharan <devishree_90@yahoo.co.in>

@devishree90 devishree90 force-pushed the devishree90:master branch from 2dc8bf1 to 36ecdbf Jan 26, 2018

@devishree90

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

@wolfs - I have added more test & updated language-scala.gradle as suggested. How do i trigger CI builds to verify my fix?

@devishree90

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

@wolfs - I see tests are still failing. Looking at following link and i dont understand exception which i see there. https://builds.gradle.org/viewLog.html?buildId=10583070&buildTypeId=Gradle_Check_SanityCheck. Am i looking at right build?

@wolfs

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

No worries, I am fixing the build and will finish this PR. I already have the your work + build fixing on a branch if you want to have a look: https://github.com/gradle/gradle/tree/wolfs/build-cache/platform-scala-compile

@devishree90

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

Thank you for the update @wolfs

@wolfs wolfs merged commit 36ecdbf into gradle:master Jan 30, 2018

2 of 7 checks passed

Performance Test Coordinator - Linux (Branch Build Accept) TeamCity build failed
Details
Branch Build Accept (Trigger) (Check) TeamCity build failed
Details
Quick Feedback (Trigger) (Check) TeamCity build failed
Details
Quick Feedback - Linux Only (Trigger) (Check) TeamCity build failed
Details
Sanity Check (Quick Feedback - Linux Only) TeamCity build failed
Details
DCO All commits have a DCO sign-off from the author
WIP ready for review
Details

wolfs added a commit that referenced this pull request Jan 30, 2018

Merge pull request #3804 from devishree90/master
Make `PlatformScalaCompile` cacheable
@wolfs

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

@devishree90 The PR has been merged and will be released as part of Gradle 4.6.
Thank you for your work and your patience.

Could you try out a nightly if caching works for your project (e.g. 4.6-20180131151911+0000)?

@wolfs

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

@devishree90 Did you have a chance to try out caching for a real world play project? I would really be interested in your results.

@devishree90

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2018

Sorry @wolfs . I totally missed your message. I tested it out. It works perfectly well. I could see 'compilePlayBinaryScala' tasks are loaded from cache. Thanks for following-up.

Let me know if you are interested in specific information.

@wolfs

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

@devishree90 Some timings would be nice. Something like how much your build takes without the build cache, and how long a fully cached build takes.

This is something we describe in the build cache guide.

@lptr lptr added the @build-cache label Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.