Strip build dependent comments in Play compilers generated files #183

jprinet · 2023-03-24T16:15:16Z

fixes #109

The generated files by the Route and Twirl compilers contain an absolute path (and a timestamp with older playframework versions) which leads to build cache misses.
The relativization in the framework assumes that the current working directory is the project directory which is not the case when using the Worker API ($HOME/.gradle/workers).

This PR removes the DATE comment and relativize the absolute path after SOURCE comment by post-processing the generated files.

…es files

jprinet · 2023-03-30T15:08:24Z

src/integTest/groovy/org/gradle/playframework/tasks/AbstractRoutesCompileIntegrationTest.groovy

@@ -121,7 +124,7 @@ GET     /newroute                          ${controllers()}.Application.index()

        and:
        assertContentsHaveChangedSince(scalaRoutesFileSnapshot, getScalaRoutesFile())
-        assertContentsHaveChangedSince(javaRoutesFileSnapshot, getJavaRoutesFile())
+        assertModificationTimeHasChangedSince(javaRoutesFileSnapshot, getJavaRoutesFile())


Removing the @(DATE) with post-processing actually makes the content identical

jprinet · 2023-03-30T15:23:15Z

@mkurz you were suggesting here to apply the same post-processing to the TwirlCompiler.
I couldn't find any evidence of an absolute path similar issue with the TwirlCompiler, could you confirm if this is required or not?

mkurz · 2023-03-30T15:36:21Z

Based on the reproducer at the bottom of #109 (comment) but running gradle compilePlayTwirlTemplates and then diffing:

diff ./target/scala-2.13/twirl/main/views/html/ ./build/src/play/twirl/views/html/

I can see that the gradle Play plugin still adds the date and the source is absolute:

/*
    -- GENERATED --
    DATE: 2023-03-30T17:31:29.276683
    SOURCE: /tmp/gradle-test/play-scala-seed/app/views/main.scala.html
    HASH: 71c20c7304dce5c18dc291baf1465b5039e08938
    MATRIX: 987->260|1111->291|1138->292|1218->397|1254->406|1289->414|1315->419|1404->481|1419->487|1482->528|1570->589|1585->595|1646->634|1709->759|1746->769|1774->776|1809->784|1850->798|1865->804|1926->844
    LINES: 26->7|31->8|32->9|35->12|36->13|36->13|36->13|37->14|37->14|37->14|38->15|38->15|38->15|42->20|43->21|43->21|45->23|45->23|45->23|45->23
    -- GENERATED --
*/

Using sbt the date is removed and the path is relative:

/*
    -- GENERATED --
    SOURCE: app/views/main.scala.html
    HASH: 52352176f1a784d96e62964e439264b508b87d6f
    MATRIX: 987->260|1111->291|1138->292|1218->397|1254->406|1289->414|1315->419|1404->481|1419->487|1482->528|1570->589|1585->595|1646->634|1709->759|1746->769|1774->776|1809->784|1850->798|1865->804|1926->844
    LINES: 26->7|31->8|32->9|35->12|36->13|36->13|36->13|37->14|37->14|37->14|38->15|38->15|38->15|42->20|43->21|43->21|45->23|45->23|45->23|45->23
    -- GENERATED --
*/

So yeah I guess you have to apply the same post processing to twirl templates like you do for the routes files.

mkurz · 2023-03-30T15:41:55Z

Hmm. I was wondering why the DATE is still present. You use an outdated twirl version:

playframework/src/main/java/org/gradle/playframework/tools/internal/twirl/TwirlCompilerFactory.java

Line 25 in 2fff17d

    
           return new TwirlCompilerAdapterV13X("1.4.2", scalaCompatibilityVersion, playTwirlAdapter);

You should upgrade to 1.5.1, I am pretty sure at least the DATE is gone, and then we could see if that also fixes the path problem.

jprinet · 2023-03-30T15:41:59Z

Alright, the impact is different in that case as an HTML file wouldn't be used as an input of a compilation task. Is it a realistic use case to have the TwirlCompiler producing some java / scala files as well?

mkurz · 2023-03-30T15:46:21Z

Is it a realistic use case to have the TwirlCompiler producing some java / scala files as well?

The twirl compiler is producing scala files. It works more or less the same like the routescompiler. The routescompiler generates mulitiple scala and java files from a foo.routes file. The twirl compiler generates scala files from a foo.scala.html file.
For caching, it will be necessary to also remove the DATE and remove or relativize the path from the generated twirl scala file.s
Exact same problem.

You should upgrade to twirl 1.5.1, independent of this problem btw.

mkurz · 2023-03-30T15:46:38Z

The above snippets are not from html files but from scala files.

jprinet · 2023-03-30T15:47:40Z

That makes sense, I'll update the PR ASAP then. Thanks @mkurz 👍

jprinet · 2023-04-03T11:29:23Z

src/main/java/org/gradle/playframework/tools/internal/twirl/TwirlCompilerFactory.java

@@ -22,7 +22,7 @@ public static VersionedTwirlCompilerAdapter createAdapter(PlayPlatform playPlatf
                return new TwirlCompilerAdapterV13X("1.3.13", scalaCompatibilityVersion, playTwirlAdapter);
            case PLAY_2_7_X:
            default:
-                return new TwirlCompilerAdapterV13X("1.4.2", scalaCompatibilityVersion, playTwirlAdapter);
+                return new TwirlCompilerAdapterV13X("1.5.1", scalaCompatibilityVersion, playTwirlAdapter);


My understanding is that there is no breaking change to address
playframework/twirl@1.5.1...1.4.2

I don't think there were breaking changes.

runningcode

just some nits, but looks great!

runningcode · 2023-04-12T11:57:11Z

src/integTest/groovy/org/gradle/playframework/tasks/AbstractRoutesCompileIntegrationTest.groovy

@@ -259,4 +262,18 @@ $ROUTES_COMPILE_TASK_NAME {
        new File(destinationDir, getReverseRoutesFileName('', '')).text.contains("extra.package")
        new File(destinationDir, getScalaRoutesFileName('', '')).text.contains("extra.package")
    }
+
+    def "post-process generated comments"() {


Suggested change

def "post-process generated comments"() {

def "post-processed generated comments contain path and timestamp replacements"() {

runningcode · 2023-04-12T12:01:09Z

src/main/java/org/gradle/playframework/tools/internal/routes/DefaultRoutesPostProcessor.java

+        String sourceReplacementString = getSourceReplacementString(spec.getSources(), spec.getProjectDir());
+
+        try (Stream<Path> stream = Files.find(spec.getDestinationDir().toPath(), Integer.MAX_VALUE, (filePath, fileAttr) -> fileAttr.isRegularFile())) {
+            stream.forEach(routeFile -> process(routeFile, sourceReplacementString));


the process function already catches the IOException. what else can throw it in this block?

Files.find does

runningcode · 2023-04-12T12:01:49Z

src/main/java/org/gradle/playframework/tools/internal/routes/DefaultRoutesPostProcessor.java

+import java.nio.file.Path;
+import java.util.stream.Stream;
+
+class DefaultRoutesPostProcessor implements Serializable {


Should we add a javadoc to clarify what exactly this class does and what it is processing?

big-guy

Thanks for doing this

big-guy · 2023-04-13T00:55:26Z

src/main/java/org/gradle/playframework/tasks/RoutesCompile.java

@@ -105,7 +107,7 @@ public ConfigurableFileCollection getRoutesCompilerClasspath() {
    @TaskAction
    @SuppressWarnings("Convert2Lambda")
    void compile() {
-        RoutesCompileSpec spec = new DefaultRoutesCompileSpec(getSource().getFiles(), getOutputDirectory().get().getAsFile(), isJavaProject(), getNamespaceReverseRouter().get(), getGenerateReverseRoutes().get(), getInjectedRoutesGenerator().get(), getAdditionalImports().get());
+        RoutesCompileSpec spec = new DefaultRoutesCompileSpec(getSource().getFiles(), getOutputDirectory().get().getAsFile(), isJavaProject(), getNamespaceReverseRouter().get(), getGenerateReverseRoutes().get(), getInjectedRoutesGenerator().get(), getAdditionalImports().get(), getProjectDir());


I think it's OK to either call getProject().getProjectDir() here or instead inject ProjectLayout and use that here.

big-guy · 2023-04-13T00:56:16Z

src/main/java/org/gradle/playframework/tasks/RoutesCompile.java

+     * @return The project directory.
+     */
+    @Internal
+    public File getProjectDir() {


Instead of exposing a new property here, I would either call getProject().getProjectDir() above or change this to:

@Inject protected abstract ProjectLayout getProjectLayout()

And use project layout above.

I directly called getProject().getProjectDir() 👍

big-guy · 2023-04-13T00:58:17Z

src/main/java/org/gradle/playframework/tools/internal/twirl/DefaultTwirlPostProcessor.java

+    // /*
+    //                    -- GENERATED --
+    //                    DATE: Mon Apr 03 10:27:51 CEST 2023
+    //                    SOURCE: /private/var/folders/79/xmc9yr493y75ptry2_nrx3r00000gn/T/junit4995996226044083355/app/views/test.scala.html


What does this look like on Windows?

Good point, let me check this

The only usage I'm aware of is this one, which is still safe to use after the PR.

big-guy · 2023-04-13T01:00:37Z

src/main/java/org/gradle/playframework/tools/internal/twirl/DefaultTwirlPostProcessor.java

+    private void process(Path generatedFile, String sourceReplacementString) {
+        try {
+            String content = new String(Files.readAllBytes(generatedFile), StandardCharsets.UTF_8);
+            String regexSource = String.format(PATTERN_MATCHER, "SOURCE");


Instead of using a regex here, would it be simpler to do something like:
For each line...

Look for GENERATED_TAG, we're in a generated section.

If we're in a generated section, does the line start with DATE or SOURCE?

If DATE, throw away the line.

If SOURCE, throw away the line and replace it with a new line

else, keep the line

Look for the GENERATED_TAG again, keep all lines after this once we see it.

I adjusted the implementation in that sense 👍

runningcode · 2023-04-24T09:45:55Z

src/main/java/org/gradle/playframework/tools/internal/routes/DefaultRoutesPostProcessor.java

+// This post processor fixes build / project dependent comments (DATE and SOURCE) from the RoutesCompiler generated files:
+// @GENERATOR:play-routes-compiler
+// @(DATE): Mon Apr 03 10:27:51 CEST 2023
+// @(SOURCE):/private/var/folders/79/xmc9yr493y75ptry2_nrx3r00000gn/T/junit4995996226044083355/conf/routes


nit: multi-line comments usually use the /** syntax.
Is 16-18 an example?

I adjusted the Javadoc comments 👍

Add configuration to strip build dependent comments in generated rout…

480e2c6

…es files

jprinet mentioned this pull request Mar 24, 2023

compileScala failed to get from cache due to compilePlayRoutes Task #109

Closed

Remove feature toggle

d7d6b85

jprinet force-pushed the feature/strip-routes-comments branch 5 times, most recently from 7036a02 to 0a0728a Compare March 30, 2023 10:58

Relativize routes files path

0b4e761

jprinet force-pushed the feature/strip-routes-comments branch from 0a0728a to 0b4e761 Compare March 30, 2023 12:46

Fix integration test relying on @Date comment presence

6c7e680

jprinet marked this pull request as ready for review March 30, 2023 15:07

jprinet requested a review from big-guy March 30, 2023 15:07

jprinet commented Mar 30, 2023

View reviewed changes

jprinet added 3 commits April 3, 2023 13:12

Relativize with RelativePathUtil

482d850

Post-process files compiled by Twirl

7da3b62

Bump up Twirl version

5e87c60

jprinet commented Apr 3, 2023

View reviewed changes

jprinet added 2 commits April 3, 2023 13:31

Adjust documentation

b96b633

Fix integration test

1cf9a7e

jprinet changed the title ~~Add configuration to strip build dependent comments in generated routes~~ Strip build dependent comments in Play compilers generated files Apr 6, 2023

runningcode reviewed Apr 12, 2023

View reviewed changes

big-guy requested changes Apr 13, 2023

View reviewed changes

jprinet added 5 commits April 24, 2023 11:01

CR review: update test name

e9f8d8f

CR review: add Javadoc

abe09b3

CR review: Do not expose projectDir as a member

3b7b0fd

CR review: Process lines by iteration instead of regex

909df3e

CR review: Remove unused import

cb91bae

runningcode reviewed Apr 24, 2023

View reviewed changes

CR review: Adjust Javadoc

2940cdd

big-guy merged commit 5f6a3d0 into gradle:master Apr 24, 2023

mkurz mentioned this pull request Oct 6, 2023

Gradle Support playframework/twirl#678

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip build dependent comments in Play compilers generated files #183

Strip build dependent comments in Play compilers generated files #183

jprinet commented Mar 24, 2023 •

edited

Loading

jprinet Mar 30, 2023

jprinet commented Mar 30, 2023

mkurz commented Mar 30, 2023 •

edited

Loading

mkurz commented Mar 30, 2023 •

edited

Loading

jprinet commented Mar 30, 2023

mkurz commented Mar 30, 2023 •

edited

Loading

mkurz commented Mar 30, 2023

jprinet commented Mar 30, 2023

jprinet Apr 3, 2023

mkurz Apr 3, 2023

runningcode left a comment

runningcode Apr 12, 2023

runningcode Apr 12, 2023

jprinet Apr 24, 2023 •

edited

Loading

runningcode Apr 12, 2023

big-guy left a comment

big-guy Apr 13, 2023

big-guy Apr 13, 2023

jprinet Apr 24, 2023

big-guy Apr 13, 2023

jprinet Apr 24, 2023

jprinet Apr 24, 2023

big-guy Apr 13, 2023

jprinet Apr 24, 2023

runningcode Apr 24, 2023

jprinet Apr 24, 2023

jprinet Apr 24, 2023

	def "post-process generated comments"() {
	def "post-processed generated comments contain path and timestamp replacements"() {

Strip build dependent comments in Play compilers generated files #183

Strip build dependent comments in Play compilers generated files #183

Conversation

jprinet commented Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

jprinet commented Mar 30, 2023

mkurz commented Mar 30, 2023 • edited Loading

mkurz commented Mar 30, 2023 • edited Loading

jprinet commented Mar 30, 2023

mkurz commented Mar 30, 2023 • edited Loading

mkurz commented Mar 30, 2023

jprinet commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runningcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jprinet Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-guy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jprinet commented Mar 24, 2023 •

edited

Loading

mkurz commented Mar 30, 2023 •

edited

Loading

mkurz commented Mar 30, 2023 •

edited

Loading

mkurz commented Mar 30, 2023 •

edited

Loading

jprinet Apr 24, 2023 •

edited

Loading