[10.6.X][GEOMETRY] Fix deps to resolve undefined reference mentioned in UBSAN IBS #25566
Conversation
|
The code-checks are being triggered in jenkins. |
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25566/7794 |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master. It involves the following packages: DetectorDescription/DDCMS @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@smuzaffar - If it's not crucial for a release, please, leave it unfixed. It'd be better to open an issue. Thanks. |
| @@ -5,6 +5,7 @@ | |||
| <use name="Geometry/RPCGeometry"/> | |||
| <use name="Geometry/DTGeometry"/> | |||
| <use name="Geometry/CSCGeometry"/> | |||
| <use name="Geometry/MTDGeometryBuilder"/> | |||
There was a problem hiding this comment.
@smuzaffar - IMHO, this is wrong. There should not be any dependencies on the geometry builders.
There was a problem hiding this comment.
The underlying problem is we need a separate package Geometry/MTDGeometry which should hold the geometry objects. I think the origin of the problem is the MTD system followed the pattern of the Tracker geometry is also needs a separate Geometry/TrackerGeometry package.
There was a problem hiding this comment.
@ianna @Dr15Jones so what is the preferred solution here? Integrate the code as it is and then modify the Tracker/MTD structure in next future, or keep this on hold until the geometries have been modified?
There was a problem hiding this comment.
I'd suggest to restructure before integrating the PR
There was a problem hiding this comment.
@ianna , this dependency is in tests only. Looks like this unit test is testing many other geometries too.
[a]
void analyzeCSC(const GlobalTrackingGeometry* geo, const CSCGeometry* cscGeometry);
void analyzeDT(const GlobalTrackingGeometry* geo, const DTGeometry* dtGeometry);
void analyzeRPC(const GlobalTrackingGeometry* geo, const RPCGeometry* rpcGeometry);
void analyzeGEM(const GlobalTrackingGeometry* geo, const GEMGeometry* gemGeometry);
void analyzeMTD(const GlobalTrackingGeometry* geo, const MTDGeometry* mtdGeometry);
void analyzeTracker(const GlobalTrackingGeometry* geo, const TrackerGeometry* tkGeometry);
| @@ -3,6 +3,7 @@ | |||
| <use name="FWCore/Utilities"/> | |||
| <use name="dd4hep"/> | |||
| <use name="rootmath"/> | |||
| <use name="rooteve"/> | |||
There was a problem hiding this comment.
@smuzaffar - I'm not sure we want to depend on Eve. I can check it next year.
There was a problem hiding this comment.
I think we need a new root package which has as its top libGeo.
There was a problem hiding this comment.
PR #25566 should resolve it.
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
please abort |
|
wrong window, sorry... |
|
Pull request #25566 was updated. @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild can you please check and sign again. |
|
@ianna , I have updated the PR. Now there is separate PR #25856 for fixing the libGeom dependency. And for this PR, the dependency is in unit tests which is testing many other geometries too ( see #25566 (comment) ) |
|
please test |
smuzaffar
changed the title
|
The tests are being triggered in jenkins. |
|
@fabiocos - if the following class is moved to the plugins directory there should not be any problem with linking against the library. @cvuosalo - FYI |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 Move of the files can be done later. |
cmsbuild
added
fully-signed
geometry-approved
and removed
geometry-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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
@fabiocos , can we get this in or should I close the PR? |
|
@smuzaffar I have tracked the problem into an issue to keep memory of this, for the time being I would say we may close this PR |
To resolve