Skip to content

Merge generated-src and generated #117899

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Merge generated-src and generated #117899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4160fa3
Merge generated-src and generated
idegtiarenko Dec 3, 2024
dc3182d
Merge branch 'refs/heads/main' into custom_generated_location
idegtiarenko Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions ...al/src/integTest/groovy/org/elasticsearch/gradle/internal/StringTemplatePluginTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,40 @@ class StringTemplatePluginTest extends AbstractGradleFuncTest {
""".stripIndent().stripTrailing()
}

def "test custom generated directory"() {
given:
internalBuild()
file('src/main/src/X-SomeFile.txt.st') << """
Just some random data
"""
file('src/main/generated/leftover.txt') << """
Just some random data
"""

buildFile << """
apply plugin: 'elasticsearch.build'
apply plugin: 'elasticsearch.string-templates'

tasks.named("stringTemplates").configure {
it.outputFolder = file("src/main/generated")
template {
it.properties = [:]
it.inputFile = file("src/main/src/X-SomeFile.txt.st")
it.outputFile = "SomeFile.txt"
}
}
"""

when:
def result = gradleRunner("stringTemplates", '-g', gradleUserHome).build()

then:
result.task(":stringTemplates").outcome == TaskOutcome.SUCCESS
file("src/main/generated/SomeFile.txt").exists()
file("src/main/generated-src/SomeFile.txt").exists() == false
file("src/main/generated/leftover.txt").exists() == false
}

def "test basic conditional"() {
given:
internalBuild()
Expand Down
4 changes: 2 additions & 2 deletions build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/StringTemplateTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ public abstract class StringTemplateTask extends DefaultTask {

@Inject
public StringTemplateTask(ObjectFactory objectFactory, FileSystemOperations fileSystemOperations) {
templateSpecListProperty = objectFactory.listProperty(TemplateSpec.class);
outputFolder = objectFactory.directoryProperty();
this.templateSpecListProperty = objectFactory.listProperty(TemplateSpec.class);
this.outputFolder = objectFactory.directoryProperty();
this.fileSystemOperations = fileSystemOperations;
}

Expand Down
11 changes: 4 additions & 7 deletions x-pack/plugin/esql/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ def projectDirectory = project.layout.projectDirectory
def generatedSourceDir = projectDirectory.dir(generatedPath)
tasks.named("compileJava").configure {
options.compilerArgumentProviders.add(new SourceDirectoryCommandLineArgumentProvider(generatedSourceDir))
// IntelliJ sticks generated files here and we can't stop it....
exclude { normalize(it.file.toString()).contains("src/main/generated-src/generated") }
}

idea.module {
Expand Down Expand Up @@ -177,8 +175,7 @@ pluginManager.withPlugin('com.diffplug.spotless') {
// for some reason "${outputPath}/EsqlBaseParser*.java" does not match the same files...
targetExclude "src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseLexer*.java",
"src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseParser*.java",
"src/main/generated/**/*.java",
"src/main/generated-src/generated/**/*.java"
"src/main/generated/**/*.java"
}
}
}
Expand Down Expand Up @@ -227,7 +224,7 @@ tasks.register("regenParser", JavaExec) {
}

tasks.register("regen") {
dependsOn "regenParser"
dependsOn "regenParser", "stringTemplates"
Copy link
Contributor Author

@idegtiarenko idegtiarenko Dec 3, 2024

Choose a reason for hiding this comment

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

I had to ensure stringTemplates runs before other code generation.

This has to be done as stringTemplates task cleans the entire generated directory before execution (see StringTemplateTask) while other generation not.

This means that before this change /generated dir was not cleaned before generating new files, keeping files with corresponding templates deleted.

Copy link
Contributor

@alex-spies alex-spies Dec 4, 2024

Choose a reason for hiding this comment

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

Are you sure this is needed? The regen task uses a different mechanism and doesn't put the output in the same dir. It makes Antlr stuff everything into x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser.

I just removed the stringTemplates dependency locally and made a small change to our grammar - the diff looked very sane and contained; whereas with the dependency on stringTemplates in place, making changes to the grammar and running regen deletes a bunch of unrelated files, which one must not commit. I think that'll make making changes to the grammar a bit painful.

Copy link
Contributor Author

@idegtiarenko idegtiarenko Dec 4, 2024

Choose a reason for hiding this comment

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

I tried to group it with other gen stuff I noticed, but may be it is worth to update the stringTemplates not to clear files unconditionally and instead have dedicated clear generated files tasks. What gradle task do we use to generate other files in main/gnerated?

Antlr generated code goes into main/java, I do not think this is intended as well. I will check If I can move it to generated in a separate pr.

doLast {
// moves token files to grammar directory for use with IDE's
ant.move(file: "${outputPath}/EsqlBaseLexer.tokens", toDir: grammarPath)
Expand Down Expand Up @@ -276,10 +273,8 @@ tasks.register("regen") {
}
}

tasks.named("spotlessJava") { dependsOn stringTemplates }
Copy link
Contributor

@alex-spies alex-spies Dec 4, 2024

Choose a reason for hiding this comment

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

Why do we remove this dependency?

Copy link
Contributor Author

@idegtiarenko idegtiarenko Dec 4, 2024

Choose a reason for hiding this comment

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

As I was explaining above stringTemplates always cleans the generation target directory before execution. If it is executed after other generation tasks it will remove their results now as they are in the same directory.

I had to remove it from here as spotlessJava is executed rather late. Instead I moved stringTemplates as early as possible.

Copy link
Contributor

@mark-vieira mark-vieira Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We cannot have multiple tasks share an output directory. If we want to have a "single" generated source directory for the purposes of the IDE then we'll need to have another task aggregate them. This should actually be a Gradle deprecation warning if you attempt to do this. I'm surprised the build didn't complain.

The issue with having to strictly order tasks because they clean their output folder is one reason why this is generally bad practice. It's very easy to have tasks clobber eachothers output.

Copy link
Contributor

@breskeby breskeby Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see any other task writing into src/main/generated in :x-pack:plugin:esql

I don't understand your explanation here why this change is needed. To me it looks like spotless is configured to ignore everything that is generated so we should be just safe to remove this task dependency as you did

tasks.named('checkstyleMain').configure {
excludes = [ "**/*.java.st" ]
exclude { normalize(it.file.toString()).contains("src/main/generated-src/generated") }
exclude { normalize(it.file.toString()).contains("src/main/generated") }
}

Expand All @@ -306,6 +301,8 @@ tasks.named('stringTemplates').configure {
var bytesRefProperties = prop("BytesRef", "BytesRef", "BYTES_REF", "org.apache.lucene.util.RamUsageEstimator.NUM_BYTES_OBJECT_REF", "")
var booleanProperties = prop("Boolean", "boolean", "BOOLEAN", "Byte.BYTES", "BitArray")

it.outputFolder = file(generatedPath)

File inInputFile = file("src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/X-InEvaluator.java.st")
template {
it.properties = booleanProperties
Expand Down
0 ...erator/comparison/InBooleanEvaluator.java → ...erator/comparison/InBooleanEvaluator.java
View file
Open in desktop
File renamed without changes.
0 ...rator/comparison/InBytesRefEvaluator.java → ...rator/comparison/InBytesRefEvaluator.java
View file
Open in desktop
File renamed without changes.
0 ...perator/comparison/InDoubleEvaluator.java → ...perator/comparison/InDoubleEvaluator.java
View file
Open in desktop
File renamed without changes.
0 ...e/operator/comparison/InIntEvaluator.java → ...e/operator/comparison/InIntEvaluator.java
View file
Open in desktop
File renamed without changes.
0 .../operator/comparison/InLongEvaluator.java → .../operator/comparison/InLongEvaluator.java
View file
Open in desktop
File renamed without changes.
Loading