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

Simplify plugin configuration #179

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

JojOatXGME
Copy link
Contributor

@JojOatXGME JojOatXGME commented Apr 1, 2024

Pull Request Details

Sorry, but I got carried away over the weekend. This PR greatly simplifies the configuration of the plugin in a backward compatible manner (although I deprecated some properties). You may pick individual changes as you which, they are separated into semantically coherent commits.

The new minimal configuration looks like this:

grammarKit {
    lexerSource = layout.projectDirectory.file("<path to *.flex file>")
    parserSource = layout.projectDirectory.file("<path to *.bnf file>")
}

This configuration is mostly equivalent to the following configuration, as it would have to be written manually before my changes:

sourceSets {
    main {
        java {
            srcDir(layout.buildDirectory.dir("generated/sources/grammarkit-lexer/java/main"))
            srcDir(tasks.generateParser)
        }
    }
}

tasks {
    generateLexer {
        sourceFile = layout.projectDirectory.file("<path to *.flex file>")
        targetOutputDir = layout.buildDirectory.dir("generated/sources/grammarkit-lexer/java/main/<pkg>")
        purgeOldFiles = true
    }

    generateParser {
        sourceFile = layout.projectDirectory.file("<path to *.bnf file>")
        targetRootOutputDir = layout.buildDirectory.dir("generated/sources/grammarkit-parser/java/main")
        pathToParser = "<relative path to parser java file>"
        pathToPsiRoot = "<relative path to psi files>"
        purgeOldFiles = true
    }

    compileJava {
        dependsOn(generateLexer)
    }
}

Description

The new, much shorter configuration is possible due to the following individual changes:

  • Allow GenerateLexerTask to create a subdirectory for the package. This allows us to give the output directory to SourceDirectorySet.srcDir directly. The new behavior is applied if the new property targetRootOutputDir is used, which replaces targetOutputDir. (362e9fc)
  • Make GenerateParserTask.pathToParser and pathToPsiRoot optional. These properties are not needed in most cases, as they are only restricting purgeOldFiles to specific sub-paths of the output directory. (05af194)
  • Make purging stale files the default. Without purging stale files, incremental builds may run into errors. When the build cache is enabled, even :clean may not be able to fix such issues. For backwards compatibility, the new behavior is only applied when the previously mentioned properties (which were mandatory) are no-longer used. (87ac815)
  • Add default values for targetRootOutputDir in both tasks. This simplifies the configuration and encourages people to use separate output directories for each task. Overlapping output directories are discouraged by Gradle and may cause issues with the build cache. (a8c60bd)
  • Introduce lexerSource and parserSource in the plugin extension. These properties make it possible to specify the source files without having to configure the individual tasks directly. When this property is used, the corresponding tasks are also added as sources to the main source set. (86022b4)

I also revised some existing tests in commit a204e86 and 5228085, but this doesn't have much impact on the other changes. I updated the changelog in commit 1a13907.

Related Issue

Motivation and Context

This change makes using the plugin much easier.

  • You no-longer have to manually configure the directory for the generated files
  • Stale files are purged by default, preventing build failures in incremental builds
  • Duplicating information from the source files in the build configuration is no-longer necessary
    • The package of the lexer
    • The package of the parser
    • The class name of the parser
    • The package containing the PSI classes
  • You no-longer have to set up task dependencies manually

How Has This Been Tested

I created various “unit” tests to test my changes. (I tripled the total amount of unit tests in this repository.) Furthermore, I tried out the new version with, and without simplifying the configuration on the nix-idea plugin.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change) – but I deprecated stuff

Checklist

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project. – Where do I get the code style?
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@JojOatXGME
Copy link
Contributor Author

I don't think the build failure on GitHub Actions is caused by my changes. The error actually looks quite familiar to me. I think I once fixed the same issue at JetBrains/gradle-changelog-plugin#170. 😅

@JojOatXGME
Copy link
Contributor Author

FYI, I also asked at gradle/gradle#9021 (comment) whether it is possible to tell Gradle to clean the output directory for you. Unfortunately, it doesn't seem to be possible right now.

@JojOatXGME JojOatXGME mentioned this pull request May 4, 2024
10 tasks
Introduce GenerateLexerTask.targetRootOutputDir as a replacement of
targetOutputDir. When using the new property, the task creates the java
sources in the directory matching the package name. The package name is
detected using some heuristics. The commit also introduces
GenerateLexerTask.packageName to manually specify the package name.
The two properties of GenerateParserTask are only used when
purgeOldFiles is explicitly set to true, but they still had to be
specified.

As of this commit, if purgeOldFiles is set to true but none of the two
properties are specified, the task will delete the whole content of
targetRootOutputDir.

I am not aware of any reason not to delete the whole directory of
targetRootOutputDir unless the directory is overlapping with other files
than the files generated by this task. Since overlapping task outputs
are explicitly discouraged by Gradle, I deprecated the two properties.
Stale files should be avoided because they can cause build failures at
subsequent tasks in incremental builds. For example removing a grammar
rule from a *.bnf file may cause a compilation error when the stale AST
note tries to call its corresponding method on the visitor.

In combination with the build cache, a stale file may even be persisted
in the cache, making :clean ineffective for fixing such issue.
The default should be sufficient in most use cases and makes the
configuration of the plugin a bit easier. Note that the default is only
available for the tasks :generateLexer and :generateParser. Custom tasks
of type GenerateLexerTask or GenerateParserTask do not have a default
for this property.
@JojOatXGME
Copy link
Contributor Author

I rebased this pull request on the current master after #187 has been merged to fix the build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make pathToParser and pathToPsiRoot optional
1 participant