Slow builds due to checkstyle

[bereng]

Thanks for sending a pull request! Here are some tips if you're new here:

• Ensure you have added or run the appropriate tests for your PR.
• Be sure to keep the PR description updated to reflect all changes.
• Write your PR title to summarize what this PR proposes.
• If possible, provide a concise example to reproduce the issue for a faster review.
• Read our contributor guidelines
• If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

[michaelsembwever]

could we just put these into the build directory instead ? or the user's tmp directory? (i don't know how it works across different clones and repos…?)

(otherwise this will break releases, e.g. deb packaging)

[bereng]

I moved it into the build dir. If it were on the tmp folder there could be cross-talk between different builds. Also let's see how this devbranch does...

[bereng]

Devbranch job seems to have been able to build ok...

[smiklosovic]

@bereng @michaelsembwever Could we make it a property in build.xml and reference it in checkstyle.xml? That way I can put whatever path I want to build.properties file. Once it is in build dir, it will be there forever no?

What if I do ant realclean jar? Even I have not changed single line of the source code, I will need to do checkstyle again?

People may have a global .gitignore in their $HOME (normal git stuff) so I can point my build to cache it somewhere it will be gitignored anyway.

[michaelsembwever]

Even I have not changed single line of the source code, I will need to do checkstyle again?

You compile classes again, even if you haven't touched them. That's the point with clean, and the behaviour i would expect.

I'm weary of having anything at the top level, it causes problems. Having it under build/ (specifically ${build.dir}) in in line with the work happening in 18133.

I would also be in favour of not calling checkstyle when jar building. With 18133, all linters are a separate command (e.g. .build/check-code.sh). To me this makes sense, as we also have owasp which is critical but we are ignoring because it's too slow (and network prone).

[bereng]

I don't see a build.properties file so I am not following. You mean you want a specific property in build.xml just for the checkstyle cache? Why would sbdy need to change that? 🤔

You need it cleaned on every 'realclean', if you're checking out branches on the same folder only realclean works and you need to rebuild the checkstyle cache. It's not perfect but it's the best compromise I came up with.

If we put it in the build folder it gets gitignored already. Look at the latest commit.

[Mmuzaf]

My understanding is that the ant realclean must clean the checkstyle cache as well, this is something much closer to the way Maven behaves and much more expected than having a dedicated checkstyle cache somewhere else. I'm +1 here.

I did small local tests by running ant build and changing some classes a few times sequentially, so that now it doesn't check the files that were not changed. Only the affected files are checked, which in turn speeds up the local build.

#1
/Users/x/IdeaProjects/cassandra/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java=1686663255220

#2
/Users/x/IdeaProjects/cassandra/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java=1686663333852

[smiklosovic]

@bereng yes, build.properties is not there but build.propeties.default is. That is supposed to be a template you place your properties in and rename it to build.properties which is in our .gitignore.

Anyway, I gave it the second thought and I think you all are right. realclean should just clean it all. We definitely need to have a target which wipes it all out regardless of the properties / settings a developer might set.

[Mmuzaf]

I have checked locally and everything is working as expected using the cacheFile property. I'm wondering if it's worth putting the cache file under the checkstyle directory ${checkstyle.log.dir} to keep everything in one place?

[Mmuzaf]

oh, I'm sorry :-) I have just seen a lot of comments related to this

[bereng]

I think you're right. I missed that folder. They belong in there imo and it will still be under build. So all good. I have committed your suggestion. Thx.

[michaelsembwever]

i know we're editing existing files here, but it really bugs me that the checkstyle xml files are checked in at the top directory.

checkstyle.xml checkstyle_report_test.xml checkstyle_test.xml should be put under .build/

[bereng]

So you want to move all the checkstyle files from build/checkstyle to .build/checkstyle? This is moving the whole checkstyle around, are you ok I do this as a separate ticket and get this merged and start enjoying it? :-)

[michaelsembwever]

So you want to move all the checkstyle files from build/checkstyle to .build/checkstyle? This is moving the whole checkstyle around, are you ok I do this as a separate ticket and get this merged and start enjoying it? :-)

No, the versioned controlled checkstyle xml files. (Not talking about the generated files.)

That is, the files checkstyle.xml, checkstyle_report_test.xml, checkstyle_test.xml should be put under .build/