[EventFilter] Move private headers in plugins dir; merge plugins cc/h files #34898

smuzaffar · 2021-08-16T17:53:20Z

To avoid private header files exposed via package interface directory, this PR proposed to move the private headers in to plugins directory.
It also merges the .cc and .h files of plugins

This is part of cleanup discussed #34718

cmsbuild · 2021-08-16T18:00:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34898/24706

This PR adds an extra 28KB to repository

cmsbuild · 2021-08-16T18:00:34Z

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master.

It involves the following packages:

EventFilter/CSCTFRawToDigi (l1)
EventFilter/EcalRawToDigi (reconstruction)

@perrotta, @cmsbuild, @rekovic, @slava77, @jpata, @cecilecaillol can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @rchatter, @argiro, @thomreis, @simonepigazzini this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

smuzaffar · 2021-08-17T05:41:25Z

please test

cmsbuild · 2021-08-17T09:30:59Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce8dbd/17814/summary.html
COMMIT: 7967ada
CMSSW: CMSSW_12_1_X_2021-08-16-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34898/17814/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 5 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000329
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-08-17T17:41:20Z

EventFilter/EcalRawToDigi/plugins/MatacqProducer.cc

@@ -18,7 +18,7 @@
 #include "DataFormats/EcalDigi/interface/EcalMatacqDigi.h"
 #include "DataFormats/FEDRawData/interface/FEDNumbering.h"
 #include "DataFormats/FEDRawData/interface/FEDRawData.h"
-#include "EventFilter/EcalRawToDigi/interface/MatacqProducer.h"
+#include "EventFilter/EcalRawToDigi/src/MatacqProducer.h"


this is a header file for class MatacqProducer : public edm::EDProducer {
We usually move them to /plugins and include without a path.
Yet a better option is to merge .h in .cc and drop it from SealModules.cc

done, I have merged the .cc and .h files for plugin and dropped SealModules.cc

… files

cmsbuild · 2021-08-18T08:36:24Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34898/24739

This PR adds an extra 48KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34898/24739/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34898/24739/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-08-18T08:44:17Z

Pull request #34898 was updated. @perrotta, @cmsbuild, @rekovic, @slava77, @jpata, @cecilecaillol can you please check and sign again.

smuzaffar · 2021-08-18T09:52:05Z

please test

cmsbuild · 2021-08-18T12:56:19Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce8dbd/17855/summary.html
COMMIT: ddde684
CMSSW: CMSSW_12_1_X_2021-08-17-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34898/17855/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS StaticAnalyzer warnings: There are 2 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce8dbd/17855/llvm-analysis/legacy-mod-sa.txt for details.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000330
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-08-19T13:39:27Z

+reconstruction

for #34898 ddde684

code changes are technical, in line with the PR description and the follow up review
jenkins tests pass and comparisons with the baseline show no relevant differences

smuzaffar · 2021-08-20T08:25:46Z

@rekovic, @cecilecaillol can you please review this?

smuzaffar · 2021-08-23T08:11:43Z

@rekovic, @cecilecaillol can you please review this?

smuzaffar · 2021-08-24T09:25:52Z

@cms-sw/l1-l2 , this is blocking the integration of build rules to enforce the private header usage check. Can you please review this?

rekovic · 2021-08-31T14:48:44Z

+1

cmsbuild · 2021-08-31T14:49:05Z

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

perrotta

A couple of duplicate includes. They could be better avoided if all includes are grouped together at the top of the file.
(Trivial fix that can also get postponed to a follow up PR)

perrotta · 2021-08-31T14:56:10Z

EventFilter/EcalRawToDigi/plugins/MatacqProducer.cc

+#include "EventFilter/EcalRawToDigi/interface/MatacqRawEvent.h"
+#include "EventFilter/EcalRawToDigi/src/MatacqDataFormatter.h"
+#include "FWCore/MessageLogger/interface/MessageLogger.h"
+#include "FWCore/Utilities/interface/InputTag.h"


perrotta · 2021-08-31T14:57:15Z

EventFilter/EcalRawToDigi/plugins/MatacqProducer.cc

+   */
+  uint32_t runNumber_;
+};
+
 #include <csignal>
 #include <cstdio>
 #include <fstream>


perrotta · 2021-09-01T05:34:31Z

+1

The duplicate includes were there even before merging the files...

smuzaffar · 2021-09-01T05:55:49Z

thanks @perrotta , I will open a PR to remove the duplicates

cmsbuild added this to the CMSSW_12_1_X milestone Aug 16, 2021

cmsbuild added code-checks-pending l1-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 16, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 16, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 17, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 17, 2021

slava77 reviewed Aug 17, 2021

View reviewed changes

[EventFilter] Move private headers in plugins dir; merge plugins cc/h…

7089fdd

… files

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Aug 18, 2021

smuzaffar changed the title ~~[EventFilter] Move private headers in to src dir~~ [EventFilter] Move private headers in plugins dir; merge plugins cc/h files Aug 18, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Aug 18, 2021

code-format

ddde684

cmsbuild added code-checks-pending and removed code-checks-rejected labels Aug 18, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 18, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 18, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 18, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Aug 19, 2021

smuzaffar mentioned this pull request Aug 23, 2021

Header files from non-interface sub-directory included #34718

Closed

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Aug 31, 2021

perrotta reviewed Aug 31, 2021

View reviewed changes

cmsbuild added orp-approved and removed orp-pending labels Sep 1, 2021

cmsbuild merged commit e5403de into cms-sw:master Sep 1, 2021

cmsbuild mentioned this pull request Sep 1, 2021

Added missing esConsumes to DTTrackFinder #35080

Merged

smuzaffar mentioned this pull request Sep 2, 2021

Remove duplicate include statements #35122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EventFilter] Move private headers in plugins dir; merge plugins cc/h files #34898

[EventFilter] Move private headers in plugins dir; merge plugins cc/h files #34898

smuzaffar commented Aug 16, 2021 •

edited

Loading

cmsbuild commented Aug 16, 2021

cmsbuild commented Aug 16, 2021

smuzaffar commented Aug 17, 2021

cmsbuild commented Aug 17, 2021

slava77 Aug 17, 2021

smuzaffar Aug 18, 2021

cmsbuild commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

smuzaffar commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

slava77 commented Aug 19, 2021

smuzaffar commented Aug 20, 2021

smuzaffar commented Aug 23, 2021

smuzaffar commented Aug 24, 2021

rekovic commented Aug 31, 2021

cmsbuild commented Aug 31, 2021

perrotta left a comment

perrotta Aug 31, 2021

perrotta Aug 31, 2021

perrotta commented Sep 1, 2021

smuzaffar commented Sep 1, 2021

[EventFilter] Move private headers in plugins dir; merge plugins cc/h files #34898

[EventFilter] Move private headers in plugins dir; merge plugins cc/h files #34898

Conversation

smuzaffar commented Aug 16, 2021 • edited Loading

cmsbuild commented Aug 16, 2021

cmsbuild commented Aug 16, 2021

smuzaffar commented Aug 17, 2021

cmsbuild commented Aug 17, 2021

Comparison Summary

slava77 Aug 17, 2021

Choose a reason for hiding this comment

smuzaffar Aug 18, 2021

Choose a reason for hiding this comment

cmsbuild commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

smuzaffar commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

Comparison Summary

slava77 commented Aug 19, 2021

smuzaffar commented Aug 20, 2021

smuzaffar commented Aug 23, 2021

smuzaffar commented Aug 24, 2021

rekovic commented Aug 31, 2021

cmsbuild commented Aug 31, 2021

perrotta left a comment

Choose a reason for hiding this comment

perrotta Aug 31, 2021

Choose a reason for hiding this comment

perrotta Aug 31, 2021

Choose a reason for hiding this comment

perrotta commented Sep 1, 2021

smuzaffar commented Sep 1, 2021

smuzaffar commented Aug 16, 2021 •

edited

Loading