-
Notifications
You must be signed in to change notification settings - Fork 41
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
Changes from 4 commits
480e2c6
d7d6b85
0b4e761
6c7e680
482d850
7da3b62
5e87c60
b96b633
1cf9a7e
e9f8d8f
abe09b3
3b7b0fd
909df3e
cb91bae
2940cdd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
=== Generated routes files comments | ||
|
||
Routes files generated by the compiler 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. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
|
||||||
|
@@ -121,7 +124,7 @@ GET /newroute ${controllers()}.Application.index() | |||||
|
||||||
and: | ||||||
assertContentsHaveChangedSince(scalaRoutesFileSnapshot, getScalaRoutesFile()) | ||||||
assertContentsHaveChangedSince(javaRoutesFileSnapshot, getJavaRoutesFile()) | ||||||
assertModificationTimeHasChangedSince(javaRoutesFileSnapshot, getJavaRoutesFile()) | ||||||
assertContentsHaveChangedSince(reverseRoutesFileSnapshot, getReverseRoutesFile()) | ||||||
|
||||||
when: | ||||||
|
@@ -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"() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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. | ||
|
@@ -52,6 +52,7 @@ public class RoutesCompile extends SourceTask { | |
private final Property<PlayPlatform> platform; | ||
private final Property<Boolean> injectedRoutesGenerator; | ||
private final ConfigurableFileCollection routesCompilerClasspath; | ||
private final File projectDir; | ||
|
||
@Inject | ||
public RoutesCompile(WorkerExecutor workerExecutor) { | ||
|
@@ -66,6 +67,7 @@ public RoutesCompile(WorkerExecutor workerExecutor) { | |
this.injectedRoutesGenerator = getProject().getObjects().property(Boolean.class); | ||
this.injectedRoutesGenerator.set(false); | ||
this.routesCompilerClasspath = getProject().files(); | ||
this.projectDir = getProject().getProjectDir(); | ||
} | ||
|
||
/** | ||
|
@@ -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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's OK to either call |
||
|
||
if (GradleVersion.current().compareTo(GradleVersion.version("5.6")) < 0) { | ||
workerExecutor.submit(RoutesCompileRunnable.class, workerConfiguration -> { | ||
|
@@ -177,4 +179,15 @@ public Property<Boolean> getGenerateReverseRoutes() { | |
public Property<Boolean> getInjectedRoutesGenerator() { | ||
return injectedRoutesGenerator; | ||
} | ||
|
||
/** | ||
* The project directory is used to relativize the route source folder | ||
* when post-processing the generated routes files. | ||
* | ||
* @return The project directory. | ||
*/ | ||
@Internal | ||
public File getProjectDir() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of exposing a new property here, I would either call
And use project layout above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I directly called |
||
return projectDir; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
package org.gradle.playframework.tools.internal.routes; | ||
|
||
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; | ||
|
||
class DefaultRoutesPostProcessor implements Serializable { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} catch (IOException e) { | ||
LOGGER.warn("Unable to post-process routes", e); | ||
} | ||
} | ||
|
||
private String getSourceReplacementString(Iterable<File> sources, File projectDir) { | ||
String sourceReplacementString = ""; | ||
|
||
if(sources.iterator().hasNext()) { | ||
File sourceFile = sources.iterator().next(); | ||
sourceReplacementString = "// @(SOURCE):" + | ||
projectDir.toURI().relativize(sourceFile.toURI()) | ||
.getPath() | ||
.replace(File.separator, "/"); | ||
} | ||
|
||
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 route file %s", routeFile.getFileName()), e); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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