HHH-18678 Use specific check tasks for CI build

[mbellade]

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion. For more information on licensing, please check here.

---

https://hibernate.atlassian.net/browse/HHH-18678

[hibernate-github-bot[bot]]

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

[mbellade]

Spotless doesn't cover every rule enforced by our code-stye, and this already caused build failures locally (where it was not disabled).

@sebersole agreed we should re-enable checkstyle just for the h2 build.

[gavinking]

Neither does checkstyle: we have lots of coding conventions which checkstyle has never enforced.

If checkstyle didn't measurably slow us down, I wouldn't mind it. But recently I lost almost half a day when the compileJava task was failing because of some checkstyle violation that wasn't being properly reported by Gradle. And even when checkstyle actually works, it's unacceptably slow on our codebase. It's just not worth dealing with this sort of crap.

[gavinking]

If you guys can fix it so that it's not pig slow, and so that the Gradle integration is nonbroken, then by all means, please go ahead and turn it back on again.

[mbellade]

Checkstyle doesn't run with the compileJava task, and this change has nothing to do with local builds. Your original changes only disabled it on the h2 run for GH actions, and this is just re-enabling it, so I'm not sure how this could affect you.

The h2 run, which already takes less then half the time then others, will simply take 2 more minutes.

[sebersole]

Part of the problem, now, with checkstyle is that it is still doing work that spotless is doing. Cleaning up the checkstyle config will definitely help. I'll change that in a little

[gavinking]

I am well-aware that this should never have happened. Nevertheless it happened. Because the Gradle integration is broken. Which is precisely why I went and spent another half-day on replacing it with spotless.

[sebersole]

And regardless of whether it is"pig slow" it is formatting we want to enforce. So how else would you suggest we do that?

And no, "simply not enforcing" is not an option.

[gavinking]

Look I agree it would be nice to enforce the brace on a new line rule. But I would also love to be able to enforce the maximum-120-characters per line rule, or the don't-stack-parens-without-a-space rule, etc, etc.

But we don't enforce those rules, nor others, and somehow we manage to live with that.

[gavinking]

So how else would you suggest we do that?

One thing we could do is push an IntelliJ config with the project, so that the IDE automatically applies the coding conventions.

[gavinking]

Alternatively, it's probably not terribly difficult to write a spotless step that replaces ^(\s+)} else with $1}\n$1else.

[marko-bekhta]

We have a code formatting step in Search and Validator builds... It actually produces a good result overall (there may be a few places with very long generics where the formatting might be a bit odd). Then in those places where we have code that goes into the documentation guides and we want to have more control, we just wrapped the code blocks with "skip-formatting-here". And these plugins have caching, so they don't need to re-run the formatting of all the files, but only those that changed, so the build time is not visibly impacted. I was looking into spotless to see if we can use it in Search/Validator and I've seen that it also has caching (https://github.com/diffplug/spotless/tree/main/plugin-maven#incremental-up-to-date-checking-and-formatting)

Sooooo .... that might also be an option to consider (add the auto formatting to spotless config and enable this incremental cache so it doesn't run through all files).

[sebersole]

One thing we could do is push an IntelliJ config with the project, so that the IDE automatically applies the coding conventions.

Like mentioned in the CONTRIBUTING?

[gavinking]

Like mentioned in the CONTRIBUTING?

Well I mean actually including the IntelliJ config in the project itself so it's loaded automatically when you open the project.

[sebersole]

Well I mean actually including the IntelliJ config in the project itself so it's loaded automatically when you open the project.

Unfortunately I don't know how to do that. Now we could move the files to import into the project itself which would be a good first step.

[mbellade]

@sebersole I commented out all checkstyle rules already enforced by spotless, also I added the spotlessCheck to the h2 build to go alongside it and disabled spotlessApply on CI since it's a bad idea to change files during the build there.

[sebersole]

I think also there may be a misundertanding here... compilation does not run checkstyle. You need to run the check lifecycle task (nothing to do with checkstyle per-se) to trigger all build time "checks".

[gavinking]

Unfortunately I don't know how to do that.

I can set it up if you like. (I used to do it this way on other projects.) The only doubt I have is I need to make sure I can properly sanitize the IntelliJ project metadata of anything specific to my environment. But I believe this to be possible.

[sebersole]

Unfortunately I don't know how to do that.

I can set it up if you like. (I used to do it this way on other projects.) The only doubt I have is I need to make sure I can properly sanitize the IntelliJ project metadata of anything specific to my environment. But I believe this to be possible.

Sure! I think that would be a great thing.

To be clear though, we still want checkstyle to run (on the H2 / "default" jobs). Even with spotless and IDE formatting enabled, it is possible for someone to send code that is not formatted. So what we discussed was to do the following on the H2 CI job -

1. execute spotlessCheck
2. execute checkstyleMain
3. disable spotlessApply

[sebersole]

@sebersole I commented out all checkstyle rules already enforced by spotless, also I added the spotlessCheck to the h2 build to go alongside it and disabled spotlessApply on CI since it's a bad idea to change files during the build there.

Did you need to explicitly disable spotlessApply? I'm surprised that is executed on a "normal CI build".

[sebersole]

@sebersole I commented out all checkstyle rules already enforced by spotless, also I added the spotlessCheck to the h2 build to go alongside it and disabled spotlessApply on CI since it's a bad idea to change files during the build there.

Did you need to explicitly disable spotlessApply? I'm surprised that is executed on a "normal CI build".

Ahh, you did -

tasks.compileJava.dependsOn spotlessApply

[gavinking]

I can set it up if you like.

See #9014 which appears to work.

[gavinking]

I think also there may be a misundertanding here... compilation does not run checkstyle.

Does publishToMavenLocal? I have to run that quite often when developing HibernateProcessor.

[sebersole]

@sebersole I commented out all checkstyle rules already enforced by spotless, also I added the spotlessCheck to the h2 build to go alongside it and disabled spotlessApply on CI since it's a bad idea to change files during the build there.

Did you need to explicitly disable spotlessApply? I'm surprised that is executed on a "normal CI build".

Ahh, you did -

tasks.compileJava.dependsOn spotlessApply

I was thinking about this some more. I think we should have a different task set up here. Something like this?

// currently spotlessApply happens every time we compile.
// here we move that to the `check` "lifecycle phase"
tasks.check.dependsOn spotlessApply

tasks.register( "ciCheck" ) {
    group "verification"
    description "Checks for most CI environments"
    dependsOn test
}

tasks.register( "ciCheckComplete" ) {
    group "verification"
    description "More complete checking for the H2 CI environment.  The extra work is only needed for one job in the group"

    dependsOn ciCheck
    dependsOn spotlessCheck
    dependsOn checkstyle
    dependsOn forbiddenApis
}

spotlessApply gets moved to check, which is what should be run locally:

• test
• spotlessApply
• checkstyle
• forbiddenApis

[gavinking]

The point of using spotless is so that it runs as soon as possible, and so you never get in a situation where you push code that hasn't had spotless run its cleanups.

[gavinking]

i.e. the thing that makes checkstyle so awful (apart from the fact that I have to go off and open up its reports in my web browser to find out what files it's complaining about) is that it is so slow that it's impossible to properly integrate into the usual edit-compile-test process. It's the mere fact that it's a separate "task" that makes it such a pain in the ass.

Spotless, on the other hand, is so fast that I don't care that it runs all the time. And by running it all the time, it becomes transparent to me.

[mbellade]

spotlessApply gets moved to check, which is what should be run locally

I'm not against this, but it increases the likelihood that users that are not running the check task explicitly won't have spotless rules applied by default. Having it on compileJava has the advantage that even compilation triggered by running tests from the IDE will apply spotless, and gradle caches the task so the impact on build-time is negligible.

I'm fine with either solution though, as eventually the rules are checked by CI anyway.

[sebersole]

CONTRIBUTING explicitly calls out the expectation that build be used locally for contributions. And considering that multiple times recently I have pulled and had spotless DoStuff when I build locally, I'd say attaching it to compile isn't having the outcome we want either.

I don't have a string opinion here, but its not like I'm alone - spotless itself made the decision to apply to the verification phase by default for Maven; it does not bind to any tasks in Gracdle by default.

[mbellade]

CONTRIBUTING explicitly calls out the expectation that build be used locally for contributions

Sure, but expecting someone to at least compile their local project before pushing should be a lot more reliable then just documentation. Also, I believe those first few files that got modified locally should just be an initial hiccup.

In any case, I've applied your suggestions @sebersole, the spotless rules will now be applied on check and will be verified on the H2 CI run. Let's hope this won't cause too many failures for users,

[gavinking]

Correct me if I'm wrong, but doesn't this change just take us directly back to exactly the bad place we started from: where I find out about code style violations after I push to GH. The only difference is that it will be spotless driving me nuts instead of checkstyle.

If so then my work in this was for nothing.