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

Move edm::ErrorSummaryEntry to DataFormats/Common #32706

Merged
merged 4 commits into from Feb 3, 2021

Conversation

Dr15Jones
Copy link
Contributor

PR description:

Keep edm::ErrorSummaryEntry for storage into the Event and create a new edm::messagelogger::ErrorSummaryEntry as part of MessageLogger. This will hopefully make it easier to migrate to ROOT's module system as we can avoid generating a dictionary for FWCore/MessageLogger.

PR validation:

The code compiles.

Splitting them in hopes of making it easier to do ROOT modules.
All edm::ErrorSummaryEntry member are in DataFormats/Common.
@Dr15Jones
Copy link
Contributor Author

@davidlange6 FYI

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32706/20825

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

DPGAnalysis/Skims
DQM/TrackingMonitor
DQMServices/Components
DataFormats/Common
FWCore/MessageLogger
FWCore/MessageService
FWCore/Modules
HLTrigger/special
PhysicsTools/UtilAlgos
RecoLocalCalo/Castor

@perrotta, @smuzaffar, @andrius-k, @Dr15Jones, @jordan-martins, @chayanit, @wajidalikhan, @fwyzard, @ErnestaP, @Martin-Grunewald, @makortel, @cmsbuild, @jfernan2, @kmaeshima, @fioriNTU, @slava77, @jpata, @santocch can you please review it and eventually sign? Thanks.
@arossi83, @barvic, @hdelanno, @makortel, @sroychow, @JanFSchulte, @rovere, @wddgit, @Martin-Grunewald, @apsallid, @fioriNTU, @jandrea, @mtosi, @idebruyn, @threus this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-780849/12426/summary.html
COMMIT: a3f4394
CMSSW: CMSSW_11_3_X_2021-01-20-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/32706/12426/install.sh to create a dev area with all the needed externals and cmssw changes.

Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

@slava77
Copy link
Contributor

slava77 commented Jan 23, 2021

why do we still need edm::messagelogger::ELseverityLevel and edm::messagelogger::ErrorSummaryEntry?
The near-identity in implementation is quite confusing.

@slava77
Copy link
Contributor

slava77 commented Jan 25, 2021

why do we still need edm::messagelogger::ELseverityLevel and edm::messagelogger::ErrorSummaryEntry?
The near-identity in implementation is quite confusing.

[in case the question got lost over the weekend]

@Dr15Jones
Copy link
Contributor Author

why do we still need edm::messagelogger::ELseverityLevel and edm::messagelogger::ErrorSummaryEntry?
The near-identity in implementation is quite confusing.

I want to keep FWCore/MessageLogger to be not dependent upon DataFormats/Common since MessageLogger is a 'low level' framework component. So edm::messagelogger::* are the objects MessageLogger uses. The edm::* ones are those which have dictionaries and can be stored.

@slava77
Copy link
Contributor

slava77 commented Jan 26, 2021

+1

for #32706 2e73826

@davidlange6
Copy link
Contributor

hi @Dr15Jones - did you consider a new DataFormats package instead of DataFormats/Common that keeps only the specific things needed by FWCOre/MessageLogger? FWCore/MessageLogger already depends on DataFormats/Provenance so it would not be a big change from today's situation.

@Martin-Grunewald
Copy link
Contributor

+1

@silviodonato
Copy link
Contributor

unassign analysis

@silviodonato
Copy link
Contributor

@cms-sw/pdmv-l2 do you have any comments?

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2021

Pull request #32706 was updated. @chayanit, @wajidalikhan, @santocch, @jordan-martins can you please check and sign again.

@chayanit
Copy link

chayanit commented Feb 2, 2021

+1

@silviodonato
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit ff9630c into cms-sw:master Feb 3, 2021
@santocch
Copy link

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing).

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.

None yet

10 participants