Add spotless check and build with tests

[eirnym]

Basic CI build for the start.

• Windows platform is omited.
• Test resuls left in the CI build unpublished
• No separation between maven plugin and gradle plugin.

@nedtwigg Could you answer a few questions for me?

1. Why we need to have a separate Windows build? I believe we just need to test API and if tool supports Windows, then we are good to go. Spotless doesn't have workers, so filename encoding is not a problem.
2. I found in all my projects, that CI results from a generic CI image is not needed in the most cases, as being reproducible locally
3. I don't see value for separation maven and gradle builds

If anything missed and needed, it can be added later on with no problem. I believe this minimal checks will cover 90% of issues.

[eirnym]

I don't quite getting why spotless check fails this way

[nedtwigg]

Thanks very much! First off, I think that

> Could not create task ':lib:spotlessJavaCheck'.
   > Could not create task ':lib:spotlessJava'.
      > No such reference 'origin/main'

Can be worked around by #710.

[jbduncan]

Why we need to have a separate Windows build?

I think it's because file paths and other things can be different between UNIX-like and Windows OSes (think UNIX's / vs Windows' \), and our plugins use files extensively, so being able to catch subtle bugs with path manipulation, different filesystems, and so on is a plus.

I'm sure that @nedtwigg can confirm either way, though.

[eirnym]

@jbduncan could Windows build be added later on? Anyway, it seems to me a test for tools we're providing a nice API wrapper.

[jbduncan]

@jbduncan could Windows build be added later on?

I think so, they're not the most common source of bugs in my experience. WDYT, @nedtwigg?

[nedtwigg]

Why we need to have a separate Windows build

@jbduncan is correct, and also we frequently broke stuff for windows users. In particular, the way we integrate with npm-based stuff is platform-specific.

CI results ... is not needed in the most cases, as being reproducible locally

Spotless gets tons of PRs that fail CI, and I almost never check them out. I take a peek at the CI to try to help get authors unstuck, and that is easier if I can tell what failed. In particular, sometimes it's just a network timeout that I can restart and it'll pass on the second go-around.

No separation between maven plugin and gradle plugin.

This is a great start, but it won't work in the long run. The tests for each are quite slow, and it's common to have PR's which don't touch one or the other. That's why the test matrix in #1472 evolved as it did.

[eirnym]

Thank you for explanation. I believe we need to start from some point and then evolve rather than do a long-writing for an ideal build system

[jbduncan]

@jbduncan could Windows build be added later on?

@jbduncan is correct, and also we frequently broke stuff for windows users. In particular, the way we integrate with npm-based stuff is platform-specific.

I stand corrected, and the Windows breakages ring a faint bell from many years ago. I must've raised an issue about such a breakage at some point. :)

...Oh, I was right! #65

[eirnym]

I see why you wanted to split the tests. I'd do that using test groups, which are easy to control from CI as well.

[eirnym]

When you merge this PR, I'll merge it back to my other PR I made today

[nedtwigg]

could Windows build be added later on

The test matrix in #1472 has a lot of tears that led to it being the shape that it is. Gating all the big long expensive tests on spotlessCheck assemble testClasses saves a lot of compute hours and gives PR authors faster feedback - a red check in thirty seconds instead of a yellow check for 30 minutes followed by a red check.

We don't have to migrate everything at once, we can migrate any subset of non-windows tests to GitHub actions, and then the rest later. But I don't want to change the structure of the tests.

we need to start from some point and then evolve

Yes! The starting point is our CircleCI config! We can evolve piece by piece, but I don't want to start from scratch with just gradle check the whole thing and tweak from there. We have a literal decade of accumulated tweaks! Each one was for a reason, and I cleaned it out before posting the spec in #1472.

[nedtwigg]

I'd do that using test groups,

I'm open to that :) Another thing to add is that I host a central Gradle buildcache. I haven't put the credentials for it into GitHub yet, but when I do that will speed things up quite a bit. But it relies on the ordering in #1472 - doing a full compile first, then running the various tests afterwards.

[eirnym]

I use build caching as you noticed, Java action have a shortcut to create such caches which rely on a hash of all build system files in the project, so caches are actually reused

[eirnym]

In all projects I have we run all tests on each commit on each branch before and after merge. This gives us a sense of security to know what's become broken.

Maybe on Circle CI cache system works differently, but on GitHub I never had a problem.

From my public projects, I have a gradle plugin which supports Java 8 to 19 (temurin and zulu - they behave differently in some cases) and Gradle 6.0 to 7.6, so I test on that matrix. Your project is quite bigger, and it contains a bit more tests to check. I understand why you test only on Java 11 and 17 with the only version of Gradle and Maven. Nevertheless, I don't see a reason to exclude checks on build.

[eirnym]

In any case, I'd add these tests, then create tests on Windows and NPM in similar way as a separate PR, then think about test separation.

17 minutes for the start - it's not a huge time to wait.

[eirnym]

PS, about why Circle CI isn't start: I believe I just have "no access" to the Circle CI you use and they have very strict separation.

[eirnym]

Just added Windows CI support. I hope it will just work, if not, I'll revert the change

[nedtwigg]

I use build caching

I think this PR is doing dependency caching. Gradle a has a specific mechanism for caching not just jars, but the output of individual tasks. It is only enabled if you add the --build-cache flag which the spec in #1472 has, but this PR does not.

By splitting the maven tests out, and using a central task caching server

spotless/settings.gradle

Lines 43 to 57 in 6865449

	remote(HttpBuildCache) {
	url = 'https://buildcache.diffplug.com/cache/'
	// but we only push if it's a trusted build (not PRs)
	String user = cred('buildcacheuser')
	String pass = cred('buildcachepass')
	if (user != null && pass != null) {
	push = true
	credentials {
	username = user
	password = pass
	}
	} else {
	credentials { username = 'anonymous' }
	}
	}

The CI builds get way faster, but it only works if you do this part from the spec : assemble_testClasses first, if it fails then nothing else starts

17 minutes for the start - it's not a huge time to wait.

You only have to wait for your PR. I have to wait for your PR and everybody else's! Making the CI faster is super super valuable to me!

I appreciate this brainstorming, and I thank you for your efforts and figuring out lots of useful stuff. I usually try to help people merge whatever is useful for them, but the CI is part of my core day-to-day workflow, the spec in #1472 has a reason for every part, it took time to spell out the spec, and it's taking so much time to justify each piece that I'm frustrated.

I'm closing this as wont merge. I'd be happy to merge a new PR that implements any subset of #1472, and this PR will definitely be useful as a template for parts of that effort. I'm always happy to answer questions, but we have surpassed a timeout on this issue - the spec is the spec.

[eirnym]

it's sad that you desided this way. I both understand your position and .. don't.

I'm closing this as wont merge

Instead of doing a review with "hey, add the --build-cache flag here", "do other particular thing there", "let's think together, how we can make your PR to the release point" and etc., I hear just "this doesn't work". It's... very constructive.

I think this PR is doing dependency caching. Gradle a has a specific mechanism for caching not just jars, but the output of individual tasks.

Yes, I do dependency caching. There's more than one way to cache things.

You only have to wait for your PR. I have to wait for your PR and everybody else's! Making the CI faster is super super valuable to me!

This is not "I can wait for my PR", it's about authority

• CircleCI is already set to run on each commit
• All CircleCI builds run in about the same time, 15-17 minutes, so nobody loss any time by interchange CircleCI to these builds.
• For every PR everybody needs an authority to state, that "this PR checks are not failing, All tests are passing and formatting is correct".
• Gradle state for Java 17 and Gradle state for Java 11 is different, and Gradle knows it well to never mix it.

Let's look on time split:

• Build time is a very small fraction in comparison to tests
• Tests definitely must be divided to grops and this CI builds just clearly show it (and it's unclear for me if this would speed up things)
• Only after test split has been done gradle state sharing becoming useful, before that point.. it's quite useless.

[eirnym]

I forgot to say about build caches between builds from different PRs. Fresh checkout will mark all gradle caches as stale.

[nedtwigg]

it's about authority

It's not about authority, it's about time. I don't want to spend more time rejustifying every lesson that we learned when building our existing test matrix. I'm willing to spend some time on that, and we have spent some time, but I don't want to spend more.

I'll be honest, I did not fully read your replies above, and this is my last comment in this PR, because I am confident I could have implemented #1472 in less time than I have already spent arguing about the spec. This PR as-is solves some problems, but it creates a new problem by setting a new baseline test matrix. I don't want a new baseline, I want to evolve the baseline we already have. I don't want to spend more of your time or my time on this topic.

[eirnym]

Thank you, I read the issue and circle ci config quite carefully.

One won't spend more time, as all tasks go in parallel instances, so overall time is quite comparable.

More CPU and memory helps Gradle a lot to compile faster. As far as I know, GitHub instances are not very tunable, as you've done in CircleCI configuration and you got quite limited CPU and memory. Maybe this could be a paid GitHub service to have better CI.

There's a few guides how to tune an ubuntu-latest and a Windows instance. However, I take them with a grain of salt.

Dependency cache is alreay save a bit of time, as build won't download dependencies twice. It's possible to try another caching, but I doubt that it'll help

Anyway, I am strictly technical and I won't spend anymore your time on this topic.