Berenguer, as I said, I started this discussion because it is confusing that we do implicit and unexpected tasks. It is inconsistent that we run checkstyle, but we skip static code analysis like Eclipse-Warnings because that actually falsifies the advantages of running checks automatically. More robust static code analysis will take even more time than Eclipse-Warnings. Eventually, nobody is guaranteed to run any Ant task before pushing if the whole development is done in IDE.
You basically want those tasks to be guaranteed to run locally before pushing, which could be addressed more consistently by adding a Git hook. Do you think we can encounter some particular problems with this approach? Maxim, this is a great idea, and I fully support it - but this does not address the issues I've raised there. - - -- --- ----- -------- ------------- Jacek Lewandowski pon., 26 cze 2023 o 14:05 Maxim Muzafarov <mmu...@apache.org> napisał(a): > Hello everyone, > > We can replace RAT with the appropriate checkstyle rule - the HeaderCheck, > I think. This will reduce the number of tools we now use and reduce the > build time as only modified files will be checked, and this, in turn, will > remove some of the concerns mentioned in the first message. > > https://checkstyle.org/apidocs/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheck.html > > > > On Mon, 26 Jun 2023 at 13:48, Berenguer Blasi <berenguerbl...@gmail.com> > wrote: > >> Just for awareness if you rebase thanks to CASSANDRA-18588 checkstyle >> shouldn't be a problem anymore. If it is still let me know and I can look >> into it. >> On 26/6/23 13:11, Jacek Lewandowski wrote: >> >> Yes, I've mentioned that there is a property we can set to skip >> checkstyle. >> >> Currently such a goal is "artifacts" which basically validates everything. >> >> >> - - -- --- ----- -------- ------------- >> Jacek Lewandowski >> >> >> pon., 26 cze 2023 o 13:09 Mike Adamson <madam...@datastax.com> >> napisał(a): >> >>> While I like the idea of this because of added time these checks take, I >>> was under the impression that checkstyle (at least) can be disabled with a >>> flag. >>> >>> If we did do this, would it make sense to have a "release" or "commit" >>> target (or some other name) that ran a full build with all checks that can >>> be used prior to pushing changes? >>> >>> On Mon, 26 Jun 2023 at 08:35, Berenguer Blasi <berenguerbl...@gmail.com> >>> wrote: >>> >>>> I would prefer sthg that is totally transparent to me and not add one >>>> more step I have to remember. Just to push/run CI to find out I missed it >>>> and rinse and repeat... With the recent fix to checkstyle I am happy as >>>> things stand atm. My 2cts >>>> On 26/6/23 8:43, Jacek Lewandowski wrote: >>>> >>>> Hi, >>>> >>>> >>>> The context is that we currently have 3 checks in the build: >>>> >>>> - Checkstyle, >>>> >>>> - Eclipse-Warnings, >>>> >>>> - RAT >>>> >>>> >>>> CheckStyle and RAT are executed with almost every target we run: build, >>>> jar, test, test-some, testclasslist, etc.; on the other hand, >>>> Eclipse-Warnings is executed automatically only with the artifacts target. >>>> >>>> >>>> Checkstyle currently uses some caching, so subsequent reruns without >>>> cleaning the project validate only the modified files. >>>> >>>> >>>> Both CI - Jenkins and Circle forces running all checks. >>>> >>>> >>>> I want to discuss whether you are ok with extracting all checks to >>>> their distinct target and not running it automatically with the targets >>>> which devs usually run locally. In particular: >>>> >>>> >>>> >>>> - "build", "jar", and all "test" targets would not trigger >>>> CheckStyle, RAT or Eclipse-Warnings >>>> - A new target "check" would trigger all CheckStyle, RAT, and >>>> Eclipse-Warnings >>>> - The new "check" target would be run along with the "artifacts" >>>> target on Jenkins-CI, and it as a separate build step in CircleCI >>>> >>>> >>>> The rationale for that change is: >>>> >>>> - Running all the checks together would be more consistent, but >>>> running all of them automatically with build and test targets could >>>> waste >>>> time when we develop something locally, frequently rebuilding and >>>> running >>>> tests. >>>> - On the other hand, it would be more consistent if the build did >>>> what we want - as a dev, when prototyping, I don't want to be forced to >>>> run >>>> analysis (and potentially fix issues) whenever I want to build a >>>> project or >>>> just run a single test. >>>> - There are ways to avoid running checks automatically by >>>> specifying some build properties. Though, the discussion is about the >>>> default behavior - on the flip side, if one wants to run the checks >>>> along >>>> with the specified target, they could add the "check" target to the >>>> command >>>> line. >>>> >>>> >>>> The rationale for keeping the checks running automatically with every >>>> target is to reduce the likelihood of not running the checks locally before >>>> pushing the branch and being surprised by failing CI soon after starting >>>> the build. >>>> >>>> >>>> That could be fixed by running checks in a pre-push Git hook. There are >>>> some benefits of this compared to the current behavior: >>>> >>>> - the checks would be run automatically only once >>>> - they would be triggered even for those devs who do everything in >>>> IDE and do not even touch Ant commands directly >>>> >>>> >>>> Checks can take time; to optimize that, they could be enforced locally >>>> to verify only the modified files in the same way as we currently determine >>>> the tests to be repeated for CircleCI. >>>> >>>> Thanks >>>> - - -- --- ----- -------- ------------- >>>> Jacek Lewandowski >>>> >>>> >>> >>> -- >>> [image: DataStax Logo Square] <https://www.datastax.com/> *Mike Adamson* >>> Engineering >>> >>> +1 650 389 6000 <16503896000> | datastax.com <https://www.datastax.com/> >>> Find DataStax Online: [image: LinkedIn Logo] >>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.linkedin.com_company_datastax&d=DwMFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=IFj3MdIKYLLXIUhYdUGB0cTzTlxyCb7_VUmICBaYilU&m=uHzE4WhPViSF0rsjSxKhfwGDU1Bo7USObSc_aIcgelo&s=akx0E6l2bnTjOvA-YxtonbW0M4b6bNg4nRwmcHNDo4Q&e=> >>> [image: Facebook Logo] >>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.facebook.com_datastax&d=DwMFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=IFj3MdIKYLLXIUhYdUGB0cTzTlxyCb7_VUmICBaYilU&m=uHzE4WhPViSF0rsjSxKhfwGDU1Bo7USObSc_aIcgelo&s=ncMlB41-6hHuqx-EhnM83-KVtjMegQ9c2l2zDzHAxiU&e=> >>> [image: Twitter Logo] <https://twitter.com/DataStax> [image: RSS >>> Feed] <https://www.datastax.com/blog/rss.xml> [image: Github Logo] >>> <https://github.com/datastax> >>> >>>
