HHH-18678 Use specific check tasks for CI build #9011

mbellade · 2024-09-25T13:13:00Z

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 · 2024-09-25T13:13:05Z

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

gavinking

Please let's not re-enable checkstyle.

The costs are way not worth the extremely marginal "benefits".

mbellade · 2024-09-25T13:30:35Z

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 · 2024-09-25T13:43:42Z

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 · 2024-09-25T13:45:36Z

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 · 2024-09-25T13:48:25Z

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 · 2024-09-25T13:51:23Z

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 · 2024-09-25T13:52:07Z

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 · 2024-09-25T13:52:34Z

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 · 2024-09-25T13:59:20Z

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 · 2024-09-25T14:09:23Z

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 · 2024-09-25T14:14:03Z

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

marko-bekhta · 2024-09-25T14:38:17Z

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 · 2024-09-25T14:39:40Z

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 · 2024-09-25T14:56:53Z

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 · 2024-09-25T16:06:34Z

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 · 2024-09-25T16:07:17Z

@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 · 2024-09-25T16:10:13Z

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 · 2024-09-25T16:11:18Z

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 · 2024-09-25T16:18:24Z

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 -

execute spotlessCheck
execute checkstyleMain
disable spotlessApply

sebersole · 2024-09-25T16:19:48Z

@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 · 2024-09-25T16:21:44Z

@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 · 2024-09-25T16:26:05Z

I can set it up if you like.

See #9014 which appears to work.

gavinking · 2024-09-25T16:33:31Z

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 · 2024-09-25T19:10:11Z

@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 · 2024-09-25T19:26:38Z

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 · 2024-09-26T15:18:27Z

Completely excluding checkstyle would be awesome.

sebersole · 2024-09-26T15:24:15Z

@gavinking disabling checkstyle on CI while it's still enabled and runs with the check task makes local builds fail, and main was in that state because we had no way of noticing it without trying locally. If we're going to do that, than either we completely exclude checkstyle from the build, or we keep it enabled somewhere it doesn't cause inconvenience to anyone (i.e. the h2 build).

We can quibble about whether spotless should be on compile or check.

However, the CI jobs absolutely should be running checkstyle and spotlessCheck and forbiddenApis. No point running those for every database of course, so doing it on the H2 job is perfectly reasonable.

gavinking · 2024-09-26T15:26:47Z

Apart from the issue of brace placement (for which we can surely find another solution), what are the critical things that checkstyle is doing, in your opinion? I mean the things that will cause the sky to fall if we don't have them?

sebersole · 2024-09-26T15:27:43Z

Completely excluding checkstyle would be awesome.

If we can get those remaining checkstyle rules handled in spotless, I 100% agree that would be better.

But if we can't, then the checkstyle checks should still be there.

sebersole · 2024-09-26T16:15:40Z

what are the critical things that checkstyle is doing, in your opinion?

MissingDeprecated
MissingOverride
EqualsHashCode
UpperEll
IllegalImport
newline at end of file handling

I'm sure some of these can be handled in spotless..

gavinking · 2024-09-26T17:14:24Z

newline at end of file handling

I already set this up in spotless, and spotless does a much better job of it, automatically adding a newline if necessary, and also stripping off unnecessary newlines.

MissingOverride

This one, if it's actually enabled, simply doesn't work. I have had to fix oodles of missing @Overrides by hand.

Anyway, the GitHub CodeQL already does a really good job of catching these: https://codeql.github.com/codeql-query-help/java/java-missing-override-annotation/

EqualsHashCode

This is nice to have.

But again, CodeQL already does it: https://codeql.github.com/codeql-query-help/java/java-inconsistent-equals-and-hashcode/

IllegalImport

I mean this is also quite nice, but there simply has to be a much more performant way to enforce this than running checkstyle! It's completely trivial to detect import illegal.package at the beginning of a line. This is a job for something more like sed.

MissingDeprecated

The value of this one seems pretty debatable because:

It only triggers when the method already has a Javadoc comment. It doesn't do anything at all if there's no Javadoc on the method.
There are only two people who regularly deprecate documented API methods: you and I, and we're both very careful about it, and we apply waaaay more rules than just this one. External contributors never add @Deprecated annotations.

UpperEll

Not counting a handful of serialVersionUids, there are 53 long literals in the whole of hibernate-core. The sky isn't going to fall if someone one day writes 1l. I have never even once in my whole career been confused by the meaning of a numeric literal in Java.

RightCurly

Just for completeness this one is definitely useful. But again it's something that you can actually do with just a regex.

Or, in summary:

2 are nice to have but can surely be done in a simpler/faster way
1 is already handled by spotless
2 are handled by CodeQL
2 we can live without (IMO)

There has gotta be some sed-like Gradle plugin out there and if there isn't we can write one.

gavinking · 2024-09-26T17:33:02Z

This works, is ridiculously fast, and reports the problem in a much nicer way than checkstyle:

task searchInFiles {
	doLast {
		def tree = fileTree("${project.projectDir}")
		tree.include "**/*.java"
		tree.each { file ->
			file.eachLine { line ->
				if (line.startsWith('import jakarta')) {
					throw new GradleException( "Illegal import in ${file}: ${line}" )
				}
			}
		}
	}
}

I'm sure we can easily improve on this.

gavinking · 2024-09-26T18:06:49Z

This seems to work quite nicely, and it really does run very fast:

task enforceRules {
	doLast {
		def illegalImport = /^import (sun|java.awt|org.slf4j).*/
		def missingNewline = /^\s*}\s*(else|catch|finally|while).*/
		def errors = 0
		def tree = fileTree("$rootDir/hibernate-core/src/main/java/")
		tree.include "**/*.java"
		tree.each { file ->
			def lineNum = 0
			def shortName = file.path.substring(rootDir.path.length())
			file.eachLine { line ->
				lineNum++
				if (line ==~ illegalImport) {
					errors++
					logger.error("Illegal import in ${shortName}\n${lineNum}: ${line}")
				}
				if (line ==~ missingNewline) {
					errors++
					logger.error("Missing newline in ${shortName}\n${lineNum}: ${line}")
				}
			}
		}
		if ( errors>0 ) {
			throw new GradleException("Code rules were violated ($errors problems)")
		}
	}
}

gavinking · 2024-09-27T08:00:50Z

See #9023.

sebersole · 2024-09-30T16:18:51Z

I pulled #9023 locally and played with it. It runs super fast. I think we should definitely move to this.

mbellade · 2024-09-30T16:25:36Z

Great, that's even better! Still, we should make sure we never call spotlessApply on CI and I would still call spotlessCheck at least on H2. Should I update this PR to that effect @sebersole (i.e. only keeping the ciCheck and ciCheckComplete tasks)?

sebersole · 2024-09-30T16:29:17Z

I would still call spotlessCheck at least on H2. Should I update this PR to that effect

I think both spotlessCheck and this new enforceRules. wdyt?

mbellade · 2024-09-30T16:33:31Z

Sure, I'll wait for Gavin's PR to be merged then I guess. I'm guessing by this change the intention is to leave spotlessApply at compile-time for local builds, which is fine by me as long as the enforceRules task works with build cache.

sebersole · 2024-09-30T18:09:49Z

In theory, if CI is enforcing this stuff correctly we should not be seeing what is happening now where we check out, compile and now end up applying someone else's bad formatting fixesg our your working tree.

as long as the enforceRules task works with build cache.

It should be. It runs before compile so to Gradle it just looks like you changed those files yourself.

Combined with better checking on the CI jobs, this should minimize the cases we've seen where a pull from upstream suddenly leads to a bunch of changed files on compile.

gradle/java-module.gradle

mbellade · 2024-10-01T08:58:38Z

gradle/java-module.gradle

+	spotlessApply {
+		enabled = false
+	}
+	spotlessJavaApply {
+		enabled = false
+	}


Only disabling spotlessApply is not enough apparently as spotlessJavaApply is run separately (run with --info for details).

sebersole · 2024-10-01T15:18:00Z

What do y'all think about making these checks a proper CI action instead (similar to Code QL)? Especially if we can make the execution of tests depend on successfully pass.

It would help make it even more obvious that its bad formatting that failed the build visually.

Note that check isn't reasonable here, imo, since that implies running tests as well. More I mean something like:

def enforceRulesTask = tasks.register("enforceRules" ) {
    ...
}

tasks.register("formatChecks" ) {
    ...

    dependsOn tasks.spotlessCheck
    dependsOn enforceRulesTask

    spotlessApply {
        enabled = false
    }
    spotlessJavaApply {
        enabled = false
    }
}

mbellade · 2024-10-02T10:22:28Z

What do y'all think about making these checks a proper CI action instead (similar to Code QL)?

I like the idea, would make it very easy to understand that the problem is related to code formatting.

Especially if we can make the execution of tests depend on successfully pass.

I'm fine with this, though note that if we want to include forbiddenApisMain it requires compilation of main files, which usually takes ~4 minutes on gh actions runner, and this would mean delaying all other build runs (that already take a long time). If we only run spotlessCheck and enforceRules however, the time to run becomes much less (50 sec to 1 minute).

mbellade · 2024-10-30T16:12:14Z

Since this PR was opened for another purpose and stale I've created a new one: #9188, that introduces a separate task for CI builds ciCheck, and also runs static code analysis (spotlessCheck and enforceRules) in a separate CI action.

gavinking requested changes Sep 25, 2024

View reviewed changes

mbellade force-pushed the checkstyle branch from 059cc35 to 5c5afae Compare September 25, 2024 15:13

mbellade changed the title ~~Re-enable checkstyle and fix code-style~~ Enable checkstyle and spotlessCheck for h2 on CI Sep 25, 2024

sebersole self-requested a review September 25, 2024 18:44

sebersole approved these changes Sep 25, 2024

View reviewed changes

gavinking mentioned this pull request Sep 27, 2024

replace checkstyle with something faster and simpler #9023

Merged

mbellade force-pushed the checkstyle branch from 1e0675a to 0d78905 Compare October 1, 2024 08:55

mbellade changed the title ~~Enable checkstyle and spotlessCheck for h2 on CI~~ HHH-18678 Use specific check tasks for CI build Oct 1, 2024

mbellade force-pushed the checkstyle branch 2 times, most recently from 29a8152 to d5bc0fb Compare October 1, 2024 08:57

mbellade commented Oct 1, 2024

View reviewed changes

mbellade force-pushed the checkstyle branch from d5bc0fb to 3a123a5 Compare October 1, 2024 09:09

mbellade force-pushed the checkstyle branch 2 times, most recently from fd80fdc to 10bbfec Compare October 2, 2024 10:03

HHH-18678 Use specific check tasks for CI build

45f23eb

mbellade force-pushed the checkstyle branch from 10bbfec to 45f23eb Compare October 2, 2024 10:21

mbellade mentioned this pull request Oct 30, 2024

HHH-18678 Specific task for CI build and separate step for format checks #9188

Merged

mbellade closed this Oct 30, 2024

Uh oh!

HHH-18678 Use specific check tasks for CI build #9011

HHH-18678 Use specific check tasks for CI build #9011

Uh oh!

Conversation

mbellade commented Sep 25, 2024 • edited by hibernate-github-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hibernate-github-bot bot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavinking left a comment

Choose a reason for hiding this comment

Uh oh!

mbellade commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

mbellade commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebersole commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

sebersole commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

marko-bekhta commented Sep 25, 2024

Uh oh!

sebersole commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

sebersole commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbellade commented Sep 25, 2024

Uh oh!

sebersole commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebersole commented Sep 25, 2024

Uh oh!

sebersole commented Sep 25, 2024

Uh oh!

sebersole commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

sebersole commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavinking commented Sep 25, 2024

Uh oh!

gavinking commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebersole commented Sep 26, 2024

Uh oh!

gavinking commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebersole commented Sep 26, 2024

Uh oh!

sebersole commented Sep 26, 2024

Uh oh!

gavinking commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mbellade commented Sep 25, 2024 •

edited by hibernate-github-bot bot

Loading

hibernate-github-bot bot commented Sep 25, 2024 •

edited

Loading

mbellade commented Sep 25, 2024 •

edited

Loading

sebersole commented Sep 25, 2024 •

edited

Loading

gavinking commented Sep 25, 2024 •

edited

Loading

sebersole commented Sep 25, 2024 •

edited

Loading

gavinking commented Sep 26, 2024 •

edited

Loading

gavinking commented Sep 26, 2024 •

edited

Loading

gavinking commented Sep 26, 2024 •

edited

Loading

gavinking commented Sep 26, 2024 •

edited

Loading

sebersole commented Oct 1, 2024 •

edited

Loading