compileScala failed to get from cache due to compilePlayRoutes Task

[aaronlijie]

When I tested the new playFramework, I found the compileScala didn't work as expect. The following is what I did and what I found:

1. I did a clean build with remote cache enabled. So the cache objects are sent to remote cache server
2. I did a clean build again. Under this case compileScala outcome should be from cache since I didn't change anything. However the outcome is success.

I found this issue might be related to compilePlayRoutes task after I analyzed the code. The is the code from PlayApplicationPlugin.java

mainScalaSourceDirectorySet.srcDir(getRoutesCompileTask(project).flatMap(task -> task.getOutputDirectory()));

The output of compilePlayRoutes is added to the scala source set. I assume the compilePlayRoutes task will call play's routesCompiler.scala. Link to the code

However play's RoutesCompiler will add source absolute path and timestamp to the generated file (Code). Finally it causes the compileScala re-run

Would you like to investigate ? Feel free to point it out if I am wrong.

[ymwang1984]

Looks that there is a RoutesSourceInfo (with an absolute path and a date, link ) getting passed along in the route compiler such as the twirls

[spingel]

I added the following snippet to my build.gradle to remove the comments from the generated route files that contain absolute paths and date stamps:

compilePlayRoutes {
    doLast {
        fileTree("${buildDir}/src/play/routes").each { file ->
            file.text = file.text.replaceAll("(?m)^// @(SOURCE|DATE):.*", "")
        }
    }
}

With that work around in place caching works as expected.

[mkurz]

This should get fixed with Play 2.8.8:
playframework/playframework#10707
playframework/twirl#378

[Sineaggi]

Is there any chance we could get an option to disable the absolute paths and date stamps generation from older versions?

Something that could post-process the generated code to remove those lines built into the plugin by default.

[jprinet]

I can confirm that the generated routes.scala files contain an absolute path (in a comment):
// @SOURCE:/<HIDDEN>/play/conf/routes

This leads to a build cache miss when the project is built from 2 different locations.

The relativization attempt assumes that the current working directory is the project directory which is not the case when using the Worker API ($HOME/.gradle/workers).

I see 2 different options:

1. Pass the project directory as a parameter to the play route compiler and use it as a base path for relativization
2. Post-process the generated route files to remove the comment containing the absolute path

Both options have cons as I would prefer the Play compiler to remain build tool agnostic on one hand and post-processing files is far from being ideal on the other hand.

Any thoughts @big-guy ?

[big-guy]

@jprinet post processing looks like the only way to me. AFAICT, those comments aren't meaningful?

Like mentioned above, before Play 2.7, there was also a datestamp.

I think we could put the post-processing in https://github.com/gradle/playframework/blob/master/src/main/java/org/gradle/playframework/tools/internal/routes/RoutesCompiler.java#L20

[jprinet]

Sounds good 👍
I'll double-check that those comments are just informative

[Sineaggi]

In our internal fork we've been using the following code

// Remove generation date, keep the source relative
try (Stream<Path> stream = Files.find(spec.getDestinationDir().toPath(), Integer.MAX_VALUE, (filePath, fileAttr) -> fileAttr.isRegularFile())) {
    String sourceRegex = getSourceRegex(spec.getSourcesRoot());
    stream.forEach(path -> {
        try {
            String source = new String(Files.readAllBytes(path), StandardCharsets.UTF_8);

            // Upstream patches
            // https://github.com/playframework/playframework/pull/10707
            // https://github.com/playframework/twirl/pull/378
            // the routes fix was back-ported to 2.8.x
            // todo: run this on only on 2.7.x and below

            // Fix from
            // https://github.com/gradle/playframework/issues/109#issuecomment-594502896
            source = source.replaceAll(sourceRegex, "");
            source = source.replaceAll("(?m)^// @(DATE):.*", "");

            Files.write(path, source.getBytes(StandardCharsets.UTF_8));
        } catch (IOException e) {
            throw new UncheckedIOException(e);
        }
    });
} catch (IOException e) {
    throw new RuntimeException("Error invoking the Play routes compiler.", e);
} catch (UncheckedIOException e) {
    throw new RuntimeException("Error invoking the Play routes compiler.", e.getCause());
}

in the RoutesCompiler class as @big-guy mentioned above.

// helper method
private static String getSourceRegex(File sourceRoot) {
    String sourceRegex = sourceRoot.getAbsolutePath();
    if (!sourceRegex.endsWith("/")) {
        sourceRegex = sourceRegex + "/";
    }
    return sourceRegex;
}

I can clean this up and throw it into a PR if you'd like.

[mkurz]

Which Play version are you guys using?

[jprinet]

Thanks, @Sineaggi for offering your help, this would definitely be appreciated.

A couple of comments on your snippet:

• I am not a Play expert but my experiences tend to demonstrate that the generated route files are fairly small in size, meaning loading the whole file to a String for the search and replace should be acceptable.
• I would write to file only in case of a pattern match
• I would add a flag to enable / disable the feature, just want to make sure that we would not have some issues with corner cases (long list of files to iterate on for instance)
• the @SOURCE comment has to be removed (in addition to @DATE)

Which Play version are you guys using?

@mkurz I was able to reproduce in OpenTelemetry build which is using 2.8.19

[Sineaggi]

@jprinet wrt @SOURCE I tried to re-implement the change in this pr playframework/twirl#378 which relativized the source files, not stripping the comment out entirely.

[jprinet]

Yes, path relativization would definitely be better 👍

[jprinet]

I eventually prepared a PR for this @Sineaggi, I can make you co-author as the replacing logic comes from your snippet.

What is the impact of simply removing SOURCE comment @mkurz, I see that it is being used?
Would using a project relative path bring some benefits?

[mkurz]

I do not understand why this is not working in Play 2.8.19. There should not be any date anymore and also the path should be relative. If the path is absolute then this is a bug in Play.
Those pull requests should have taken care of that already:

• Routes compiler: Remove generation date, keep the source relative playframework/playframework#10707
• Routes compiler: Remove generation date, keep the source relative (bp #10707) playframework/playframework#10735 (made it into 2.8.8)
• Remove the generation date, keep the source relative playframework/twirl#378

[jprinet]

The DATE is only present with older versions of the framework indeed.
The absolute path for SOURCE is still present because the relativization uses the current working directory (.) as reference assuming it is a parent of the route file, however with Gradle Worker API, current working directory is $HOME/.gradle/workers.

[mkurz]

So I checked again, once using sbt and once using gradle.
Using Play 2.8.19 // @DATE is not added anymore, also it is totally safe to just remove it, it has no real use.

Regarding // @SOURCE:

• When using sbt the path is relativ
• Using gradle the path is absolute.

That is because of this line:

new File(".").toURI.relativize(task.file.toURI).getPath.replace(File.separator, "/"),

Assuming the project is located in /home/mkurz/projects/play-scala-seed/:
task.file.toURI always is (no matter if using sbt or gradle) file:/home/mkurz/projects/play-scala-seed/conf/routes.

But:

• Using sbt new File(".").toURI is the folder of the project (e.g. file:/home/mkurz/projects/play-scala-seed/)
• Using gradle it is file:/home/mkurz/.gradle/workers/

That is why when using gradle relativize will return the absolute path.

The// @SOURCE: line is used (at least in sbt) to map a line within a generated file to its origin line in its origin file in case of errors (so a user sees the line of the original file even if the error was caused and thrown technically by the generated file).
I am not really sure if the gradle play plugin benefits from this mechanism, if not then it is totally safe to just remove the line.

However, the best solution probably is to also "relativize" the path instead of removing the line completly.

Also, if you need help from Play side, what I can offer is that you pass the current project's folder via an Java property (e.g. gradle.play.project.home) and Play could use that so it generates relative urls even when gradle is used.
This is what I mean:

new File(sys.props.get("gradle.play.project.home").getOrElse(".")).toURI.relativize(task.file.toURI).getPath.replace(File.separator, "/"),

This way the only patch you need is basically just one line, like settings the java property around here and here just before you call the compile methods.

this is how I tested (click to expand)

$ sbt new playframework/play-scala-seed.g8 
...
// using default values
...

Template applied in ./play-scala-seed

$ cd play-scala-seed
./play-scala-seed$ sbt compile
...
[info] compiling 7 Scala sources and 1 Java source to ./play-scala-seed/target/scala-2.13/classes ...
[success] Total time: 4 s, completed Mar 24, 2023, 9:37:29 PM

./play-scala-seed$ vim build.gradle # Adding the gradle build
./play-scala-seed$ gradle compilePlayRoutes

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/8.0.2/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 855ms
1 actionable task: 1 executed

# Now you can diff the two folders ./target/scala-2.13/routes/main/ ./build/src/play/routes/
./play-scala-seed$ kompare ./target/scala-2.13/routes/main/ ./build/src/play/routes/
./play-scala-seed$ meld ./target/scala-2.13/routes/main/ ./build/src/play/routes/

And this is what my build.gradle looked like:

$ cat build.gradle 
plugins {
    id 'org.gradle.playframework' version '0.13'
}

repositories {
    mavenCentral()
    maven {
        name "lightbend-maven-release"
        url "https://repo.lightbend.com/lightbend/maven-releases"
    }
    ivy {
        name "lightbend-ivy-release"
        url "https://repo.lightbend.com/lightbend/ivy-releases"
        layout "ivy"
    }
}

play {
    platform {
        playVersion = '2.8.19'
        scalaVersion = '2.13'
        javaVersion = JavaVersion.VERSION_1_8
    }
}

[Sineaggi]

Would it make sense to have the convention configured to true by default if the playVersion is below 2.8?

[mkurz]

@Sineaggi ..below 2.8.8...

[Sineaggi]

Similar to the existing code

platform.getPlayVersion().map(playVersion -> !PlayMajorVersion.forPlayVersion(playVersion).hasSupportForStaticRoutesGenerator()

maybe something like

stripRoutesComments.convention(
platform.getPlayVersion().map(playVersion -> VersionNumber.parse(playVersion).compareTo(VersionNumber.parse("2.8.8")) >= 0);
);

[jprinet]

@mkurz Having this relativization fixed in the Play framework would certainly be better than post-processing files. The fix wouldn't be available to already released versions, but I don't see this as critical.

Adding a system property would have some impact on caching as it can be part of the cache key. What about an additional optional parameter to the routes compiler?
Any thoughts on this @big-guy?

If we decide to go for a post-processing approach, and as this is not a critical fix, I would still make it an opt-in rather than having it true below 2.8.8 @Sineaggi.

[big-guy]

Adding a system property would have some impact on caching as it can be part of the cache key. What about an additional optional parameter to the routes compiler?

This is all internal between the Gradle task and the Play code, so caching wouldn't be impacted by this. We can set system properties or call new constructor for RoutesCompilerTask. We'd create a new RoutesCompilerAdapter that would only kick in for versions of Play that support the new constructor.

If we decide to go for a post-processing approach, and as this is not a critical fix, I would still make it an opt-in rather than having it true below 2.8.8 .

We shouldn't make this opt-in because it's just a foot gun. I don't think it's going to matter if it's removed because we don't do the remapping like @mkurz mentions AFAIK.

If we want to be closer to the 2.8.8+ behavior, we could rewrite the line instead of removing it. This is also what mkurz suggested. We know the file path and could know the path to make it relative to.

[jprinet]

Sounds good, let's opt for the post-processing approach, it has the benefit of addressing all framework versions and being self-contained in the plugin.
I'll remove the feature toggle as per your suggestion, a quick lookup on Github shew that most of the route files are hundreds of line long, and longest I found was 1.6K lines which sound reasonable.

I'll adjust the @SOURCE comment to use a relative path.

[mkurz]

So nothing you need from Play side. Have a nice day 😉