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

Strip build dependent comments in Play compilers generated files #183

Merged
merged 15 commits into from
Apr 24, 2023
Merged
4 changes: 3 additions & 1 deletion src/docs/asciidoc/10-plugin-conventions.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ include::13-tasks.adoc[]

include::14-source-sets.adoc[]

include::15-dependency-configurations.adoc[]
include::15-dependency-configurations.adoc[]

include::16-generated-files-comments.adoc[]
4 changes: 4 additions & 0 deletions src/docs/asciidoc/16-generated-files-comments.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
=== Generated files comments

Files generated by Twirl and Route compilers contain comments which are changing across builds (absolute path and date depending on the framework version), this prevents tasks using those files as inputs to benefit from build cache.
The plugin is post-processing those files to remove timestamp and convert absolute paths to relative paths.
4 changes: 4 additions & 0 deletions src/docs/asciidoc/50-changes.adoc
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
== Change Log

[discrete]
=== v0.14 (TBD)
* Add configuration to post-process routes comments ({uri-github-issues}/109[issue #109]).

[discrete]
=== v0.13 (2023-01-24)
* Remove usage of internal Gradle API org.gradle.api.internal.artifacts.dsl.LazyPublishArtifact
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ import org.gradle.testkit.runner.TaskOutcome
import org.gradle.util.VersionNumber
import org.junit.Assume

import java.nio.charset.StandardCharsets

import static org.gradle.playframework.fixtures.Repositories.playRepositories
import static org.gradle.playframework.fixtures.file.FileFixtures.assertContentsHaveChangedSince
import static org.gradle.playframework.fixtures.file.FileFixtures.assertModificationTimeHasChangedSince
import static org.gradle.playframework.fixtures.file.FileFixtures.snapshot
import static org.gradle.playframework.plugins.PlayRoutesPlugin.ROUTES_COMPILE_TASK_NAME

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

and:
assertContentsHaveChangedSince(scalaRoutesFileSnapshot, getScalaRoutesFile())
assertContentsHaveChangedSince(javaRoutesFileSnapshot, getJavaRoutesFile())
assertModificationTimeHasChangedSince(javaRoutesFileSnapshot, getJavaRoutesFile())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertContentsHaveChangedSince(reverseRoutesFileSnapshot, getReverseRoutesFile())

when:
Expand Down Expand Up @@ -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-processed generated comments contain path and timestamp replacements"() {
given:
withRoutesTemplate()
when:
build(ROUTES_COMPILE_TASK_NAME)
then:
createRouteFileList().each {
def generatedFile = new File(destinationDir, it)
assert generatedFile.isFile()
assert generatedFile.getText(StandardCharsets.UTF_8.toString()).contains("// @(SOURCE):conf/routes")
assert !generatedFile.getText(StandardCharsets.UTF_8.toString()).contains("// @(DATE)")
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class TwirlCompileIntegrationTest extends PlayMultiVersionIntegrationTest {
BuildResult result = build(SCALA_COMPILE_TASK_NAME)
then:
result.task(TWIRL_COMPILE_TASK_PATH).outcome == TaskOutcome.SUCCESS
result.task(SCALA_COMPILE_TASK_PATH).outcome == TaskOutcome.SUCCESS
result.task(SCALA_COMPILE_TASK_PATH).outcome == TaskOutcome.UP_TO_DATE
}

def "can specify additional imports for a Twirl template"() {
Expand Down Expand Up @@ -290,6 +290,26 @@ class TwirlCompileIntegrationTest extends PlayMultiVersionIntegrationTest {
result.output.contains("Twirl compiler could not find a matching template for 'test.scala.custom'.")
}

@Unroll
def "post-process generated comments"() {
given:
twirlTemplate("test.scala.${format}") << template
when:
build(SCALA_COMPILE_TASK_NAME)
then:
def generatedFile = new File(destinationDir, "${format}/test.template.scala")
generatedFile.isFile()
generatedFile.text.contains("SOURCE: views/test.scala.${format}")
!generatedFile.text.contains("DATE:")

where:
format | templateFormat | template
"js" | 'JavaScriptFormat' | '@(username: String) alert(@helper.json(username));'
"xml" | 'XmlFormat' | '@(username: String) <xml> <username> @username </username>'
"txt" | 'TxtFormat' | '@(username: String) @username'
"html" | 'HtmlFormat' | '@(username: String) <html> <body> <h1>Hello @username</h1> </body> </html>'
}

def withTemplateSource(File templateFile) {
templateFile << """@(message: String)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ final class FileFixtures {
assertNotEquals(oldSnapshot.hash, now.hash)
}

static void assertModificationTimeHasChangedSince(Snapshot oldSnapshot, File file) {
Snapshot now = snapshot(file)
assertNotEquals(oldSnapshot.modTime, now.modTime)
}

private static File assertIsFile(File file) {
assertTrue(file.isFile())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import org.gradle.api.Action;
import org.gradle.api.file.ConfigurableFileCollection;
import org.gradle.api.file.Directory;
import org.gradle.api.file.DirectoryProperty;
import org.gradle.api.file.FileTree;
import org.gradle.api.provider.ListProperty;
Expand All @@ -29,6 +28,7 @@
import org.gradle.workers.WorkerExecutor;

import javax.inject.Inject;
import java.io.File;

/**
* Task for compiling routes templates into Scala code.
Expand All @@ -52,7 +52,6 @@ public class RoutesCompile extends SourceTask {
private final Property<PlayPlatform> platform;
private final Property<Boolean> injectedRoutesGenerator;
private final ConfigurableFileCollection routesCompilerClasspath;

@Inject
public RoutesCompile(WorkerExecutor workerExecutor) {
this.workerExecutor = workerExecutor;
Expand Down Expand Up @@ -105,7 +104,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(), getProject().getProjectDir());

if (GradleVersion.current().compareTo(GradleVersion.version("5.6")) < 0) {
workerExecutor.submit(RoutesCompileRunnable.class, workerConfiguration -> {
Expand Down Expand Up @@ -177,4 +176,5 @@ public Property<Boolean> getGenerateReverseRoutes() {
public Property<Boolean> getInjectedRoutesGenerator() {
return injectedRoutesGenerator;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,17 @@ public class DefaultRoutesCompileSpec implements RoutesCompileSpec {
private final boolean generateReverseRoutes;
private final boolean injectedRoutesGenerator;
private final Collection<String> additionalImports;
private final File projectDir;

public DefaultRoutesCompileSpec(Iterable<File> sourceFiles, File outputDirectory, boolean javaProject, boolean namespaceReverseRouter, boolean generateReverseRoutes, boolean injectedRoutesGenerator, Collection<String> additionalImports) {
public DefaultRoutesCompileSpec(Iterable<File> sourceFiles, File outputDirectory, boolean javaProject, boolean namespaceReverseRouter, boolean generateReverseRoutes, boolean injectedRoutesGenerator, Collection<String> additionalImports, File projectDir) {
this.sourceFiles = sourceFiles;
this.outputDirectory = outputDirectory;
this.javaProject = javaProject;
this.namespaceReverseRouter = namespaceReverseRouter;
this.generateReverseRoutes = generateReverseRoutes;
this.injectedRoutesGenerator = injectedRoutesGenerator;
this.additionalImports = additionalImports;
this.projectDir = projectDir;
}

@Override
Expand Down Expand Up @@ -56,4 +58,9 @@ public boolean isInjectedRoutesGenerator() {
public Collection<String> getAdditionalImports() {
return additionalImports;
}

@Override
public File getProjectDir() {
return projectDir;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package org.gradle.playframework.tools.internal.routes;

import org.gradle.util.RelativePathUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.File;
import java.io.IOException;
import java.io.Serializable;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.stream.Stream;

// 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted the Javadoc comments 👍

class DefaultRoutesPostProcessor implements Serializable {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private static final Logger LOGGER = LoggerFactory.getLogger(DefaultRoutesPostProcessor.class);

void execute(RoutesCompileSpec spec) {
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));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jprinet jprinet Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files.find does

} catch (IOException e) {
LOGGER.warn("Unable to post-process files", e);
}
}

private String getSourceReplacementString(Iterable<File> sources, File projectDir) {
String sourceReplacementString = "";

if(sources.iterator().hasNext()) {
File sourceFile = sources.iterator().next();
sourceReplacementString = "// @(SOURCE):" + RelativePathUtil.relativePath(projectDir, sourceFile);
}

return sourceReplacementString;
}

private void process(Path routeFile, String sourceReplacementString) {
try {
String content = new String(Files.readAllBytes(routeFile), StandardCharsets.UTF_8);
content = content.replaceAll("(?m)^// @(SOURCE):.*", sourceReplacementString);
content = content.replaceAll("(?m)^// @(DATE):.*", "");
Files.write(routeFile, content.getBytes(StandardCharsets.UTF_8));
} catch (IOException e) {
LOGGER.warn(String.format("Unable to post-process file %s", routeFile.getFileName()), e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,6 @@ public interface RoutesCompileSpec extends PlayCompileSpec, Serializable {
boolean isInjectedRoutesGenerator();

Collection<String> getAdditionalImports();

File getProjectDir();
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@
public class RoutesCompiler implements Compiler<RoutesCompileSpec>, Serializable {
private final VersionedRoutesCompilerAdapter adapter;

private final DefaultRoutesPostProcessor postProcessor;

public RoutesCompiler(VersionedRoutesCompilerAdapter adapter) {
this.adapter = adapter;
this.postProcessor = new DefaultRoutesPostProcessor();
}

@Override
Expand Down Expand Up @@ -42,6 +45,9 @@ public WorkResult execute(RoutesCompileSpec spec) {
didWork = ret || didWork;
}

// Post-process routes files
postProcessor.execute(spec);

return WorkResults.didWork(didWork);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package org.gradle.playframework.tools.internal.twirl;

import org.gradle.api.internal.file.RelativeFile;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.io.Serializable;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Stream;

// This post processor fixes build / project dependent comments (DATE and SOURCE) from the TwirlCompiler generated files:
// -- GENERATED --
// DATE: Mon Apr 03 10:27:51 CEST 2023
// SOURCE: /private/var/folders/79/xmc9yr493y75ptry2_nrx3r00000gn/T/junit4995996226044083355/app/views/test.scala.html
// HASH: 4bbbe5fde39afa0d46da8df7714a136a78170d6a
// MATRIX: 728->1|841->19|869->20|920->45|948->53
// LINES: 21->1|26->1|26->1|26->1|26->1
// -- GENERATED --
class DefaultTwirlPostProcessor implements Serializable {

private static final Logger LOGGER = LoggerFactory.getLogger(DefaultTwirlPostProcessor.class);

private static final String GENERATED_TAG = "-- GENERATED --";
private static final String GENERATED_LINE_PREFIX_DATE = "DATE: ";
private static final String GENERATED_LINE_PREFIX_SOURCE = "SOURCE: ";

void execute(TwirlCompileSpec spec) {
String sourceReplacementString = getSourceReplacementString(spec.getSources());

try (Stream<Path> stream = Files.find(spec.getDestinationDir().toPath(), Integer.MAX_VALUE, (filePath, fileAttr) -> fileAttr.isRegularFile())) {
stream.forEach(routeFile -> process(routeFile, sourceReplacementString));
} catch (IOException e) {
LOGGER.warn("Unable to post-process files", e);
}
}

private String getSourceReplacementString(Iterable<RelativeFile> sources) {
String sourceReplacementString = "";

if(sources.iterator().hasNext()) {
RelativeFile sourceFile = sources.iterator().next();
sourceReplacementString = "SOURCE: " + sourceFile.getRelativePath();
}

return sourceReplacementString;
}

private void process(Path generatedFile, String sourceReplacementString) {
try {
List<String> generatedSourceLines = Files.readAllLines(generatedFile, StandardCharsets.UTF_8);
List<String> updatedSourceLines = new ArrayList<>();

boolean isInGeneratedSection = false;
for (String currentLine : generatedSourceLines) {
if(currentLine.contains(GENERATED_TAG)) {
isInGeneratedSection = !isInGeneratedSection;
}
if(isInGeneratedSection && currentLine.contains(GENERATED_LINE_PREFIX_SOURCE)) {
// update path to relative and keep trailing spaces
String updatedLine = currentLine.substring(0, currentLine.indexOf(GENERATED_LINE_PREFIX_SOURCE)) + sourceReplacementString;
updatedSourceLines.add(updatedLine);
} else if(!(isInGeneratedSection && currentLine.contains(GENERATED_LINE_PREFIX_DATE))) {
updatedSourceLines.add(currentLine);
}
}

Files.write(generatedFile, updatedSourceLines, StandardCharsets.UTF_8);
} catch (IOException e) {
LOGGER.warn(String.format("Unable to post-process file %s", generatedFile.getFileName()), e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,11 @@ public class TwirlCompiler implements Compiler<TwirlCompileSpec>, Serializable {

private final VersionedTwirlCompilerAdapter adapter;

private final DefaultTwirlPostProcessor postProcessor;

public TwirlCompiler(VersionedTwirlCompilerAdapter adapter) {
this.adapter = adapter;
this.postProcessor = new DefaultTwirlPostProcessor();
}

@Override
Expand All @@ -48,6 +51,9 @@ public WorkResult execute(TwirlCompileSpec spec) {
}
}

// Post-process files
postProcessor.execute(spec);

return WorkResults.didWork(!outputFiles.isEmpty());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there were breaking changes.

}
}

Expand Down