sherter / google-java-format-gradle-plugin

MIT License
189 stars 35 forks source link

Delayed writing of results to build/.../fileStates.txt #35

Open danwallach opened 5 years ago

danwallach commented 5 years ago

I'm building an autograder for my class, and I'm planning to use your tool to ensure students' code is properly formatted. My problem is that I need to parse fileStates.txt so I can give a summary in the autograder (e.g., "8/9 files are formatted; run gradle googleJavaFormat to fix this"). So far as I can tell, here's your relevant code:

    private FileInfoStore setupFileStore(FileInfoStore store, FileToStateMapper mapper) {
        project.gradle.buildFinished {
            try {
                store.update(mapper)
            } catch (IOException e) {
                project.getLogger().error("Failed to write formatting states to disk", e)
            } finally {
                store.close()
            }
        }
        return store
    }

I think my problem is that you're delaying the write until the build finishes, and I need this information as part of the same build. I'm not an expert on the guts of Gradle, but I think you want to do this after the project is finished, or just right away, rather than waiting until the entire build has finished.

sherter commented 5 years ago

Hi, sorry for not getting back to you earlier.

First of all, the fileStates.txt file is by no means an API. It is used as a cache to speed up consecutive runs of format/verify tasks. The file's format can change at any time (for example, to something binary, like a SQLite or H2 database). I started out with a plain text file mainly for development convenience/ease of debugging.

I'm not sure I understand what you are trying to do. If you really want to grade some student's project, you can't use the result of running verifyGoogleJavaFormat inside that project, can you? I mean, what if he simply removed all the inputs from the task (exclude '*')? I think you should use google-java-format itself (not this plugin) and iterate over all the *.java files in the project directory. Or maybe I don't get the problem correctly...

As for your suggestion to persist the cache earlier in the build lifecycle, I'm not aware of any hook that allows me to do stuff after all tasks of a project were executed. It might be possible to somehow analyze the TaskExecutionGraph and sneak in some action to execute after the last format/verify task of a project, but I'd rather not introduce a hack in order to support another hack.

danwallach commented 5 years ago

What I'm trying to do: Write an autograder which looks at all the ways a student could make mistakes and assign points to them. You can play with it if you want:

https://github.com/RiceComp215-Staff/RiceChecks

The top-level idea is that students do their work with GitHub. On every push, Travis-CI runs ./gradlew autograde, which then builds everything, runs all the tests, assigns points, and prints out the results. Human graders come along afterward to to make sure there were no shenanigans (e.g., hacking the build.gradle file).

I want to run everything, no matter what, and then collect up all the errors, rather than Gradle's usual policy of stopping the first time it finds a problem. As such, for code formatting, I've rigged up a task to run your plugin like so:

import com.github.sherter.googlejavaformatgradleplugin.VerifyGoogleJavaFormat
task autograderVerifyGoogleJavaFormat(type: VerifyGoogleJavaFormat) {
    source 'src/main'
    source 'src/test'
    include '**/*.java'
    ignoreFailures true
}

The autograder then looks for your fileStates.txt file, and similar artifacts left behind by CheckStyle, JaCoCo, and the JUnit test runner, to arrive at a student's grade. The only task that doesn't leave behind any evidence in the filesystem turns out to be running the Java compiler itself. For that, I have Gradle redirect stdout to a file.

Certainly, I could rig up my autograder to directly run GoogleJavaFormat, but your plugin seems to do everything I care about, except for delaying its writing of fileStates.txt until the very end of the build, which forced me to also have the autograder run then, which generates a "deprecated feature" warning from Gradle.

I suppose I could try to capture your stdout like I do with the Java compiler.

sherter commented 5 years ago

I see. Generating reports for VerifyGoogleJavaFormat tasks certainly makes sense. However, this feature would be orthogonal to the fileStates cache. The cache isn't task specific. It's shared between all verify/format tasks (see https://github.com/sherter/google-java-format-gradle-plugin/blob/cc0aba8fdc462d52674585d4bdccb7933e823000/src/main/groovy/com/github/sherter/googlejavaformatgradleplugin/GoogleJavaFormatPlugin.groovy#L33-L46) and therefore not representative as a results report for one specific task.

We would need a properly defined schema for the report. What information should go in there? XML? JSON? What's the default file name pattern? We should probably have a look at all the other plugins that somehow generate reports and do it in a similar fashion.

I like the idea, but can't say if and when I will have time to implement it. If you are interested in contributing, I would be happy to comment on ideas and review code.

danwallach commented 5 years ago

All the other tools I'm looking at produce XML and HTML reports: one for the humans and one for the machines. Let's focus on the machine-readable for now. The seemingly obvious thing is to produce XML that follows the same basic schema as the CSV that you're generating now: a list of entries, one per file, with various attributes for each file.

I don't know Groovy well enough to know if it's easier or harder to do JSON vs XML. On the other side, parsing the report, it's all the same, at least using a tool like Jackson.

The question is where the right place is to generate the report. I'm willing to take a swing at it, but I'm not sure where it best fits, e.g., whether report generation should be an option on the verify task or whether it should be its own task. JaCoCo has a separate task; Checkstyle doesn't.

sherter commented 5 years ago

In my conception a report belongs to a verification task and therefore I would say that VerifyGoogleJavaFormat should implement Reporting<>. I haven't really used the JaCoCo plugin though and don't know what the rational behind having a separate task is.

danwallach commented 5 years ago

For them, "reporting" can generate a huge directory tree of HTML, one per source file, so I can see the justification for breaking that out into a separate action. For your plugin, that's not particularly necessary.