Closed fabiocos closed 4 years ago
A new Issue was created by @fabiocos Fabio Cossutti.
@Dr15Jones, @smuzaffar, @silviodonato, @makortel, @davidlange6, @fabiocos can you please review it and eventually sign/assign? Thanks.
cms-bot commands are listed here
assign core
New categories assigned: core
@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks
So should we consider building occasionally (e.g. once a week?) with EDM_ML_DEBUG
?
@makortel I think it might help. This issue is not breaking any production workflow, but shows that we integrate dysfunctional (although mostly unused) code
@fabiocos , @silviodonato , as discussed, we will add an extra test for IBs (production arch only) to compile with this flag. We will also update the PR testsing job to parse the static analyzer scram log (where we enable this flag) and report any build errors
@casarsa there are two errors in MTD digitization:
#ifdef EDM_ML_DEBUG
for (int it = 0; it < (int)(chargeColl.size()); it++)
debug |= (chargeColl[it] > adcThreshold_fC_);
#endif
but adcThreshold_fC_
is no more there. Can we just replace it with 0
? Or what is your suggestion?
@casarsa there are two errors in MTD digitization:
#ifdef EDM_ML_DEBUG for (int it = 0; it < (int)(chargeColl.size()); it++) debug |= (chargeColl[it] > adcThreshold_fC_); #endif
but
adcThreshold_fC_
is no more there. Can we just replace it with0
? Or what is your suggestion?
@fabiocos 0 is fine
@casarsa wait, you have now adcThreshold_MIP_
, should we just use that? It looks to me that this is the case...
@fabiocos you are right adcThresholdMIP is used to set a bit in the BTL and ETL samples. For consistency, it's better to use this constant.
according to my test on CMSSW_11_1_X_2020-03-31-1100 the PR #29283 has fixed all the know problems. @smuzaffar @silviodonato I would say that if a warning may be implemented, now it could be done on top of a clean situation.
Thank you @fabiocos!
@fabiocos , @silviodonato I have opened https://github.com/cms-sw/cms-bot/pull/1289 which will look for build errors in the static analyzer build log. It will create a new link on the PR summary page ( https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b8b54e/5623/summary.html ) . Currently bot will not reject PR in case of static analyzer build error though. If no objections then I will merge cms-sw/cms-bot#1289 it.
By the way, I noticed that we do not run static analyzer for Fireworks
packages and test
subdirectory
SCRAM_IGNORE_PACKAGES="Fireworks/% Utilities/StaticAnalyzers"
SCRAM_IGNORE_SUBDIRS=test
So we are not checking full cmssw for EDM_ML_DEBUG
thanks @smuzaffar
@smuzaffar thanks, this is indeed the kind of check that can be useful. The fact that Fireworks is left out implies that this is a partial test, but as far as I can see Fireworks is not using this flag, and it is already having a kind of special treatment in PR checks.
Trying to use some LogDebug statements I have realized that we have code associated to them, apparently not tested in the regular integration procedure because the needed flag is not active, that looks broken.
Ad far as I can see in cms-bot,
EDM_ML_DEBUG
seems to be used only in the static analyzer test (@smuzaffar do you confirm ?).For future reference I have compiled the whole CMSSW as of CMSSW_11_1_X_2020-03-19-1100 with
USER_CXXFLAGS="-g -D=EDM_ML_DEBUG" scram b -k
and see the following list of errors to be inspected: