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

[Core] Mark cms::Exception and edm::Exception functions that throw exceptions as noreturn #46388

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

iarspider
Copy link
Contributor

PR description:

This should hopefully solve LLVM analyzer warnings like this: link

PR validation:

Bot tests

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 15, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @iarspider for master.

It involves the following packages:

  • FWCore/Utilities (core)

@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@felicepantaleo, @makortel, @missirol, @wddgit this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@iarspider
Copy link
Contributor Author

test parameters:

  • cms-addpkg = DataFormats/Common

@iarspider
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: Build
Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-730ac3/42197/summary.html
COMMIT: 72a24e2
CMSSW: CMSSW_14_2_X_2024-10-15-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46388/42197/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation warning when building: See details on the summary page.

@iarspider
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #46388 was updated. @Dr15Jones, @makortel, @smuzaffar can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-730ac3/42219/summary.html
COMMIT: 1637e96
CMSSW: CMSSW_14_2_X_2024-10-15-1400/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46388/42219/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #46388 was updated. @Dr15Jones, @makortel, @smuzaffar can you please check and sign again.

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-730ac3/42251/summary.html
COMMIT: 17fb372
CMSSW: CMSSW_14_2_X_2024-10-16-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46388/42251/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 1 errors in the following unit tests:

---> test test_SiStripG2GainsValidator had ERRORS

Comparison Summary

Summary:

  • You potentially added 3 lines to the logs
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 44
  • DQMHistoTests: Total histograms compared: 3335129
  • DQMHistoTests: Total failures: 419
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3334690
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
  • Checked 193 log files, 163 edm output root files, 44 DQM output files
  • TriggerResults: no differences found

@iarspider
Copy link
Contributor Author

@cmsbuild ignore tests-rejected with ib-failure
Failing unit test should be fixed with #46414

@makortel
Copy link
Contributor

Comparison differences are related to #46416

@makortel
Copy link
Contributor

+core

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (test failures were overridden). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @antoniovilela, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2)

@makortel
Copy link
Contributor

The clang compilation warning collector missed

Do we know why the warning wasn't flagged by the PR test machinery?

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 2f9b38f into cms-sw:master Oct 18, 2024
11 of 12 checks passed
@smuzaffar
Copy link
Contributor

The clang compilation warning collector missed

Do we know why the warning wasn't flagged by the PR test machinery?

@makortel , bot flags only those warnings which are coming from files touched by the PR. In this case warning was coming from FWCore/Utilities/src/ExceptionCollector.cc which was not touched by this PR. We can update bot to report warning if file which has the warning is directly or in-directly using the files touched by PR ... does this sound reasonable?

@makortel
Copy link
Contributor

The clang compilation warning collector missed

Do we know why the warning wasn't flagged by the PR test machinery?

@makortel , bot flags only those warnings which are coming from files touched by the PR. In this case warning was coming from FWCore/Utilities/src/ExceptionCollector.cc which was not touched by this PR. We can update bot to report warning if file which has the warning is directly or in-directly using the files touched by PR ... does this sound reasonable?

Sounds good, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants