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
Add missing build dependencies and update checksums #6984
Conversation
A new Pull Request was created by @wmtan for CMSSW_7_4_X. Add missing build dependencies and update checksums It involves the following packages: DataFormats/CaloTowers @civanch, @nclopezo, @mdhildreth, @cmsbuild, @StoyanStoynev, @slava77, @mulhearn can you please review it and eventually sign? Thanks. |
@cmsbuild please test |
<version ClassVersion="10" checksum="3608243239"/> | ||
</class> | ||
<class name="RPCCompDetId" ClassVersion="-1"> | ||
<class name="RPCCompDetId" ClassVersion="0"> | ||
<version ClassVersion="0" checksum="2407084845"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dr15Jones Chris, I have no idea about negative or "0" class versions - I thought we always need >1, could you comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think root uses 0 to mean never store. We don't use 0 and instead have an explicit key we use to denote transient (which I don't remember off the top of my head). I don't think negatives are allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update the pull request to fix this. scram b updateclassversion blindly adds 1, so it added 1 to -1 and got 0. I didn't notice this in this branch, although I did fix it in the ROOT6 branch.
I asked Philippe about "-1". I don't remember exactly what it means, but it means something like
"use the TClass* as the checksum". It does not mean "do not store", as 0 does.
So "0" is wrong here. I will change the "0" to "10", as it is in the ROOT6 branch and update the pull request. This is what Philippe said whas the right thing to do.
@cmsbuild please test |
Pull request #6984 was updated. @civanch, @nclopezo, @mdhildreth, @cmsbuild, @StoyanStoynev, @slava77, @mulhearn can you please check and sign again. |
+1 |
Bypassing L1 approval. @mulhearn complain if not ok. The current setup is obviously broken. |
Add missing build dependencies and update checksums
Do not port this request to CMSSW_7_4_ROOT6_X. This problem will be fixed separately there.
Many DataFormats packages containing classes inheriting from DetId were missing declared link dependencies on DataFormats/DetID.
In ROOT 5.34, and ROOT 6, checksums depend on the base class.
Because of the missing link dependency, the other packages were built before DataFormats/DetId.
Because of this, edmCheckClassVersion calculated the wrong checksum. The problem with edmClassVersion needs to be fixed separately, because the checksum it calculates can depend on build order of the packages.
This pull request supplies the missing dependencies, and updates the checksums accordingly, using scram b updateclassversion.
Please expedite this critical pull request, bypassing signatures if not signed in a timely manner.
However, do not bypass testing, comparisons, and all that!!