search

cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.07k stars 4.29k forks source link

move "ClangBuild" to code checks? #37950

Open VinInn opened 2 years ago

VinInn commented 2 years ago

I suggest to move "ClangBuild" to code checks pass.

cmsbuild commented 2 years ago

A new Issue was created by @VinInn Vincenzo Innocente.

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

makortel commented 2 years ago

assign core

cmsbuild commented 2 years ago

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

makortel commented 2 years ago

@VinInn Could you explain what gains are you looking for with that?

VinInn commented 2 years ago

It's a check of code syntax. Avoids one starts the tests and then discovers that has to modify something "trivial" in the code.

makortel commented 2 years ago

I find it a bit strange that clang-tidy or clang-format would not catch syntax issues. Are you perhaps referring to https://github.com/cms-sw/cmssw/pull/37887#issuecomment-1126691241 as a concrete example? Since the issue there were warnings along

/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_4_X_2022-05-13-2300/src/HeterogeneousCore/CUDAUtilities/interface/cudaMemoryPool.h:88:42: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]

I can imagine our clang-tidy/clang-format tooling would not report those.

My main concern is increasing the time it takes to run code checks for all PRs (that in some cases can be annoyingly long already now). Maybe running just a compilation pass (no linking) and not checking out dependent packages might be lightweight enough. Or writing out just assembly (e.g. into /devnull`), or something else, to run only the frontend to speed it up more.

On the other hand, it seems to me that the fraction of PRs that get warnings only from clang is not large (but I have selection bias). Not checking out dependent packages would also limit the cases that would get tested at this stage (e.g. non-instantiated templates).

@smuzaffar What do you think?

VinInn commented 2 years ago

Yes I submitted this issue after getting the -Wpessimizing-move. Sorry for not having quote it previously.

smuzaffar commented 2 years ago

clang-format does not catch any syntax issue but for clang-tidy code should be clean (buildable) so it should catch any syntax errors. code-checks part of PR tests just makes sure that the code you have touched has the proper format and passes clang-tidy checks. For this purpose there is no need to checkout the dependenct packages.