Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove codechecker external #6118

Merged
merged 1 commit into from Jul 30, 2020
Merged

Remove codechecker external #6118

merged 1 commit into from Jul 30, 2020

Conversation

mrodozov
Copy link
Contributor

@mrodozov mrodozov commented Jul 28, 2020 •
edited

please test

the codechecker external is part of llvm but it was copied from older version of llvm and now every time
we change llvm we need also to adjust it, although we already are using it from the llvm for cmssw code-checks and format.
last time this adjustment took me long enough until I figured this is already part of llvm.
this externals is also the only reason (IIRC) why we can't conclude #5627 - it has custom cmake config to make it a standalone tool (although it's a part of llvm) which needs to be redone entirely so to work with llvm if we conclude #5627

no other external is using it nor any part of cmssw, so it should not break anything

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 28, 2020 •
edited

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mrodozov (Mircho Rodozov) for branch IB/CMSSW_11_2_X/master.

@cmsbuild, @smuzaffar, @mrodozov, @tulamor can you please review it and eventually sign? Thanks.
cms-bot commands are listed here

@smuzaffar
Copy link
Contributor

@gartung , can you please confirm if we are using it and if there are any objections on dropping it?

@gartung
Copy link
Member

gartung commented Jul 28, 2020

It is needed for https://github.com/cms-externals/CMSCodeChecker.
You need to check with @makortel if we should drop this since it was never used.

@makortel
Copy link
Contributor

In general I'm fine with dropping stuff that was never used. Is the (intended) functionality of CMSCodeChecker essentially included in the static analyzer (that is inside CMSSW)?

@Dr15Jones, do you have any thoughts?

@gartung
Copy link
Member

gartung commented Jul 28, 2020 •
edited

CMSCodeChecker was a clang-formatter that rewrites getByToken with get in place. I had it working for every case in the code base. It was never integrated because there was an objection (from David Lange?) to keeping a modified copy of the clang code-checker subsystem.

@gartung
Copy link
Member

gartung commented Jul 28, 2020

If the problem is fixing it for new versions of clang I can do this.

@davidlange6
Copy link
Contributor

davidlange6 commented Jul 28, 2020 via email

@gartung
Copy link
Member

gartung commented Jul 28, 2020 via email •
edited

@gartung
Copy link
Member

gartung commented Jul 28, 2020

It was only ~2 months development time on my part so feel free to abandon it.

@mrodozov
Copy link
Contributor Author

If the thing is used I'm fine with keeping it there two problems with it:

  • needs interface adjustment, which was frustrating until I figured out it's part of clang
  • it's cmake config needs to be redone so that it works with shared libs llvm requested here: LLVM distribution #5627 for which I have to rebuild it again and see what was not working (it was something that cmake that describes what llvm libs, headers, structure etc should be so that it would config itself, based on the llvm cmake config)

There was the benefit of reading it and trying to understand how it works that once you start understand it you can write your own checks. Not that I learned it but I recall that was the general idea of it

@davidlange6
Copy link
Contributor

davidlange6 commented Jul 28, 2020 via email

@makortel
Copy link
Contributor

I believe the functionality of rewriting specific functions calls could be handy in the future.

(did we actually run the getByToken() -> get() on the CMSSW? I can't remember)

@gartung
Copy link
Member

gartung commented Jul 28, 2020

Looking at the changes @mrodozov made for llvm9, it seems like it might be better to just add the one source file to the llvm/clang tree and build it there.
cms-externals/CMSCodeChecker@master...mrodozov:changes-for-llvm9

@davidlange6
Copy link
Contributor

davidlange6 commented Jul 28, 2020 via email

@gartung
Copy link
Member

gartung commented Jul 28, 2020

That was what @Dr15Jones asked me to implement. It was never run and committed. I had it running on every example in the code at the time. Is the getByToken() -> get() migration happening now?

@gartung
Copy link
Member

gartung commented Jul 28, 2020

@mrodozov please ask if you have problems with the interface changes. I could have saved you some frustrations.

@makortel
Copy link
Contributor

That was what @Dr15Jones asked me to implement. It was never run and committed. I had it running on every example in the code at the time. Is the getByToken() -> get() migration happening now?

No, there is no migration going on (little point in doing it manually). I guess the this checker fell through the cracks, and we should think it again. Ideally the tool should be used exactly along clang-tidy (in PR checks and automated code migrations).

@gartung
Copy link
Member

gartung commented Jul 28, 2020

Looking at the tests I see some of the warnings expected from the checker

warning: direct call of function getByToken(EDGetTokenT<>&, Handle<>&) is deprecated and should be replaced here with handleVar = iEvent.getHandle(token)
warning: bool return call of function getByToken(EDGetTokenT<>&, Handle<>&) is deprecated and should be replaced here with bool(m_handle = iEvent.getHandle(m_token))
warning: direct call of function getByToken(EDGetTokenT<>&, Handle<>&) is deprecated and should be replaced here with handleVar4 = iEvent.getHandle(*ptoken)

This code check will rewrite the code as suggested --> getByToken is replace by getHandle

@gartung
Copy link
Member

gartung commented Jul 28, 2020

If we can figure out how to add this source to our fork of llvm/clang, I am OK with removing the stand alone checker and toolfile.

@mrodozov
Copy link
Contributor Author

abort

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9478a1/8383/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2525444
  • DQMHistoTests: Total failures: 11
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2525385
  • DQMHistoTests: Total skipped: 47
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 33 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 144 log files, 17 edm output root files, 34 DQM output files

@gartung
Copy link
Member

gartung commented Jul 29, 2020

@mrodozov go ahead and merge this to remove the standalone clang-tidy.

@gartung
Copy link
Member

gartung commented Jul 29, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 29, 2020 •
edited

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+1
Tested at: 76c7248
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0cd095/8400/summary.html
CMSSW: CMSSW_11_2_X_2020-07-28-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0cd095/8400/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2525444
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2525396
  • DQMHistoTests: Total skipped: 47
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 144 log files, 17 edm output root files, 34 DQM output files

@smuzaffar
Copy link
Contributor

+externals

@smuzaffar smuzaffar merged commit 992ebe5 into cms-sw:IB/CMSSW_11_2_X/master Jul 30, 2020
@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IB/CMSSW_11_2_X/master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@mrodozov
Copy link
Contributor Author

@mrodozov go ahead and merge this to remove the standalone clang-tidy.

sorry I saw this too late. anyway, it's merged

@mrodozov mrodozov deleted the remove-codechecker-external branch July 30, 2020 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
comparison-available externals-approved fully-signed orp-pending tests-approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mrodozov @cmsbuild @smuzaffar @gartung @makortel @davidlange6