[DQM] Cleanup for private header usage #34773
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34773/24438
|
|
A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master. It involves the following packages:
@andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @jfernan2, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-33d024/17533/summary.html Comparison SummarySummary:
|
|
Thanks @smuzaffar !! |
|
+1 |
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-pending
pending-signatures
labels
|
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. @silviodonato, @dpiparo, @qliphy, @perrotta (and backports should be raised in the release meeting by the corresponding L2) |
| @@ -0,0 +1 @@ | |||
| #include "DQMServices/Core/src/ROOTFilePB.pb.h" | |||
There was a problem hiding this comment.
@smuzaffar is this "trick" according to the rules? Isn't it the same as including from DQMServices/Core/src?
Why not to move directly DQMServices/Core/src/ROOTFilePB.pb.h into \interface?
There was a problem hiding this comment.
yes trick is according to the rules i.e. within a package one can use private headers :-) This allows unit tests of a package to use private headers and check the functionality.
I have no objections on moving ROOTFilePB.pb.h to interface too. We just need to make sure that next time when we update protobuf and re-generate these ROOTFilePB.pb.* files then the ROOTFilePB.pb.h should go in to interface.
Any objections @cms-sw/dqm-l2 ?
There was a problem hiding this comment.
No objections as far as I can tell
There was a problem hiding this comment.
yes trick is according to the rules i.e. within a package one can use private headers :-) This allows unit tests of a package to use private headers and check the functionality.
Yes, but when included from DQMServices/FileIO/plugins/DQMFileSaverPB.cc or DQMServices/StreamerIO/plugins/DQMProtobufReader.cc it is not "within a package" any more...
There was a problem hiding this comment.
thats true @perrotta but build rules only look for direct include of a private header of other package. As DQMServices/FileIO/plugins/DQMFileSaverPB.cc is not directly including src/ROOTFilePB.pb.h so it will not fail. Anyway, I will update the PR to move ROOTFilePB.pb.h in to interface
There was a problem hiding this comment.
done, src/ROOTFilePB.pb.h moved to interface/ROOTFilePB.pb.h
cmsbuild
added
code-checks-pending
dqm-pending
pending-signatures
tests-pending
and removed
fully-signed
tests-approved
labels
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34773/24481 |
|
Pull request #34773 was updated. @andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @jfernan2, @rvenditti can you please check and sign again. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-33d024/17559/summary.html Comparison SummarySummary:
|
|
@cms-sw/dqm-l2 any further comment? |
|
+1 |
cmsbuild
added
dqm-approved
fully-signed
and removed
dqm-pending
pending-signatures
labels
|
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) |
|
+1 |
As reported in #34718 , this PR cleans up private header usage from
DQMDQM/Physics/src/EwkDQM.hincludeDQMServices/Core/src/ROOTFilePB.pb.his a generated header which we generate fromROOTFilePB.proto. I would suggest to keep it on theDQMServices/Core/srcand instead provide a wrapperDQMServices/Core/interface/ROOTFilePB.hwhich other packages can include.