Clean DQMOffline/MuonDPG/BuildFile.xml and remove UBSAN comments #36855
Conversation
At a few places, there are comments in Build files that a given dependency is "needed for UBSAN builds. I'd argue that this these comments are confusing an should be removed: if a package has a dependency it's because headers from that dependency are included, so no need to argue with the UBSAN builds. Both in the `CalibTracker/SiStripHitEfficiency` and `DQM/TrackerRemapper` BuildFiles, the packages indeed include headers from `DataFormats/Scalers`, so arguing with UBSAN builds is completely redundant. A good example for why these comments are confusing was the BuildFile in `DQMOffline/MuonDPG` that package doesn't actually depend on `DataFormats/Scalers` as far as I can tell, but because the BuildFile was copy-pasted from `DQM/TrackerRemapper` it included the comment about UBSAN builds which was not even applicable. That's why I didn't clean that BuildFile in #36834, but instead make this separate commit to elaborate the arguments.
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36855/28096
|
|
A new Pull Request was created by @guitargeek (Jonas Rembser) for master. It involves the following packages:
@malbouis, @civanch, @yuanchao, @makortel, @cvuosalo, @emanueleusai, @ianna, @mdhildreth, @tvami, @cmsbuild, @AdrianoDee, @Dr15Jones, @jfernan2, @ahmad3213, @pmandrik, @srimanob, @francescobrivio, @pbo0, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test for CMSSW_12_3_UBSAN_X |
|
These comments were added by @dan131riley in #34102, presumably on behalf of the core framework group. Perhaps the author could comment on the intentions |
|
UBSAN builds sometimes need typeinfo for types that are forward declared. The places where I added dependencies with the comment about UBSAN builds, those modules built fine with forward references without those dependencies declared, except in UBSAN builds. If those modules now have an explicit dependency on the types that they previously just forward declared, then removing the UBSAN comment is fine, but to be clear--there are cases where UBSAN needs to see the typeinfo for classes that have only been forward declared, not included. |
|
Hi @dan131riley, thanks for clarifying and supporting the case that the comments can be removed if there are direct dependencies! Just for the record, the direct dependencies are here: In |
Just wondering when these developed (supposedly after the comments were added)? were they hidden by other includes that got eventually cleaned? |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-61d5d6/22134/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Comparison SummarySummary:
|
|
+alca
|
Some of them were added later, some of them were actually already there before. I didn't investigate the history so much because what matters is that these includes are there now, right? |
Well, just trying to see why the ones that were there before did compile outside of |
|
Ah, I see what you mean! I also don't know exactly why the UBSAN builds notice these missing dependencies while the regular builds don't |
|
Thanks a lot for the explanation! |
|
please test for CMSSW_12_3_UBSAN_X |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-61d5d6/22183/summary.html Comparison SummarySummary:
|
|
+1 |
|
+1 |
|
+Upgrade Technical PR. |
|
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) |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
|
+1 |
PR description:
Note: this should be tested like
please test for CMSSW_12_3_UBSAN_X(see #36834 (comment))At a few places, there are comments in Build files that a given
dependency is "needed for UBSAN builds". I'd argue that this these
comments are confusing an should be removed: if a package has a
dependency it's because headers from that dependency are included, so no
need to argue with the UBSAN builds.
Both in the
CalibTracker/SiStripHitEfficiencyandDQM/TrackerRemapperBuildFiles, the packages indeed include headersfrom
DataFormats/Scalers, so arguing with UBSAN builds is completelyredundant.
A good example for why these comments are confusing was the BuildFile in
DQMOffline/MuonDPGthat package doesn't actually depend onDataFormats/Scalersas far as I can tell, but because the BuildFilewas copy-pasted from
DQM/TrackerRemapperit included the comment aboutUBSAN builds which was not even applicable. That's why I didn't clean
that BuildFile in #36834, but instead make this separate commit to
elaborate the arguments.
PR validation:
CMSSW compiles.
if this PR is a backport please specify the original PR and why you need to backport that PR:
No backport intended.