Skip to content
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

Refactor edmCheckClassVersion #45610

Merged
merged 2 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 106 additions & 95 deletions FWCore/Reflection/scripts/edmCheckClassVersion
Original file line number Diff line number Diff line change
Expand Up @@ -46,99 +46,110 @@ def checkDictionaries(name):

return missingDict

#Setup the options
from argparse import ArgumentParser, ArgumentDefaultsHelpFormatter
oparser = ArgumentParser(formatter_class=ArgumentDefaultsHelpFormatter)
oparser.add_argument("-d","--check_dictionaries", dest="checkdict",action="store_true",default=False,
help="check that all required dictionaries are loaded")
oparser.add_argument("-l","--lib", dest="library", type=str,
help="specify the library to load. If not set classes are found using the PluginManager")
oparser.add_argument("-x","--xml_file", dest="xmlfile",default="./classes_def.xml", type=str,
help="the classes_def.xml file to read")
oparser.add_argument("-g","--generate_new",dest="generate", action="store_true",default=False,
help="instead of issuing errors, generate a new classes_def.xml file.")

options=oparser.parse_args()

ClassesDefUtils.initROOT(options.library)
if options.library is None and options.checkdict:
print ("Dictionary checks require a specific library")

missingDict = 0

ClassesDefUtils.initCheckClass()

try:
p = ClassesDefUtils.XmlParser(options.xmlfile)
except RuntimeError as e:
print(f"Parsing {options.xmlfile} failed: {e}")
sys.exit(1)
foundErrors = dict()
for name,info in p.classes.items():
errorCode,rootClassVersion,classChecksum = ClassesDefUtils.checkClass(name,info[ClassesDefUtils.XmlParser.classVersionIndex],info[ClassesDefUtils.XmlParser.versionsToChecksumIndex])
if errorCode != ClassesDefUtils.noError:
foundErrors[name]=(errorCode,classChecksum,rootClassVersion)
if options.checkdict :
missingDict += checkDictionaries(name)

foundRootDoesNotMatchError = False
originalToNormalizedNames = dict()
for name,retValues in foundErrors.items():
origName = p.classes[name][ClassesDefUtils.XmlParser.originalNameIndex]
originalToNormalizedNames[origName]=name
code = retValues[0]
classVersion = p.classes[name][ClassesDefUtils.XmlParser.classVersionIndex]
classChecksum = retValues[1]
rootClassVersion = retValues[2]
if code == ClassesDefUtils.errorRootDoesNotMatchClassDef:
foundRootDoesNotMatchError=True
print ("error: for class '"+name+"' ROOT says the ClassVersion is "+str(rootClassVersion)+" but classes_def.xml says it is "+str(classVersion)+". Are you sure everything compiled correctly?")
elif code == ClassesDefUtils.errorMustUpdateClassVersion and not options.generate:
print ("error: class '"+name+"' has a different checksum for ClassVersion "+str(classVersion)+". Increment ClassVersion to "+str(classVersion+1)+" and assign it to checksum "+str(classChecksum))
elif not options.generate:
print ("error:class '"+name+"' needs to include the following as part of its 'class' declaration")
print (' <version ClassVersion="'+str(classVersion)+'" checksum="'+str(classChecksum)+'"/>')


if options.generate and not foundRootDoesNotMatchError and not missingDict:
f = open(options.xmlfile)
outFile = open('classes_def.xml.generated','w')
out = ''
for l in f.readlines():
newLine = l
if -1 != l.find('<class') and -1 != l.find('ClassVersion'):
splitArgs = l.split('"')
name = splitArgs[1]
normName = originalToNormalizedNames.get(name,None)
if normName is not None:
indent = l.find('<')
#this is a class with a problem
classVersion = p.classes[normName][XmlParser.classVersionIndex]
code,checksum,rootClassVersion = foundErrors[normName]
hasNoSubElements = (-1 != l.find('/>'))
if code == ClassesDefUtils.errorMustUpdateClassVersion:
classVersion += 1
parts = splitArgs[:]
indexToClassVersion = 0
for pt in parts:
indexToClassVersion +=1
if -1 != pt.find('ClassVersion'):
break
parts[indexToClassVersion]=str(classVersion)
newLine = '"'.join(parts)

if hasNoSubElements:
newLine = newLine.replace('/','')
out +=newLine
newLine =' '*indent+' <version ClassVersion="'+str(classVersion)+'" checksum="'+str(checksum)+'"/>\n'
if hasNoSubElements:
out += newLine
newLine=' '*indent+'</class>\n'
out +=newLine

outFile.writelines(out)

if (len(foundErrors)>0 and not options.generate) or (options.generate and foundRootDoesNotMatchError) or missingDict:
import sys
sys.exit(1)
def checkClassDefinitions(classes, checkdict):
missingDict = 0
foundErrors = dict()
for name,info in classes.items():
errorCode,rootClassVersion,classChecksum = ClassesDefUtils.checkClass(name,info[ClassesDefUtils.XmlParser.classVersionIndex],info[ClassesDefUtils.XmlParser.versionsToChecksumIndex])
if errorCode != ClassesDefUtils.noError:
foundErrors[name]=(errorCode,classChecksum,rootClassVersion)
if checkdict :
missingDict += checkDictionaries(name)
return (missingDict, foundErrors)

def checkErrors(foundErrors, classes, generate):
foundRootDoesNotMatchError = False
originalToNormalizedNames = dict()
for name,retValues in foundErrors.items():
origName = classes[name][ClassesDefUtils.XmlParser.originalNameIndex]
originalToNormalizedNames[origName]=name
code = retValues[0]
classVersion = classes[name][ClassesDefUtils.XmlParser.classVersionIndex]
classChecksum = retValues[1]
rootClassVersion = retValues[2]
if code == ClassesDefUtils.errorRootDoesNotMatchClassDef:
foundRootDoesNotMatchError=True
print ("error: for class '"+name+"' ROOT says the ClassVersion is "+str(rootClassVersion)+" but classes_def.xml says it is "+str(classVersion)+". Are you sure everything compiled correctly?")
elif code == ClassesDefUtils.errorMustUpdateClassVersion and not generate:
print ("error: class '"+name+"' has a different checksum for ClassVersion "+str(classVersion)+". Increment ClassVersion to "+str(classVersion+1)+" and assign it to checksum "+str(classChecksum))
elif not generate:
print ("error:class '"+name+"' needs to include the following as part of its 'class' declaration")
print (' <version ClassVersion="'+str(classVersion)+'" checksum="'+str(classChecksum)+'"/>')
return (foundRootDoesNotMatchError, originalToNormalizedNames)


def generate(oldfile, newfile, originalToNormalizedNames, classes, foundErrors):
with open(oldfile) as f, open(newfile, 'w') as outFile:
out = ''
for l in f.readlines():
newLine = l
if -1 != l.find('<class') and -1 != l.find('ClassVersion'):
splitArgs = l.split('"')
name = splitArgs[1]
normName = originalToNormalizedNames.get(name,None)
if normName is not None:
indent = l.find('<')
#this is a class with a problem
classVersion = classes[normName][ClassesDefUtils.XmlParser.classVersionIndex]
code,checksum,rootClassVersion = foundErrors[normName]
hasNoSubElements = (-1 != l.find('/>'))
if code == ClassesDefUtils.errorMustUpdateClassVersion:
classVersion += 1
parts = splitArgs[:]
indexToClassVersion = 0
for pt in parts:
indexToClassVersion +=1
if -1 != pt.find('ClassVersion'):
break
parts[indexToClassVersion]=str(classVersion)
newLine = '"'.join(parts)

if hasNoSubElements:
newLine = newLine.replace('/','')
out +=newLine
newLine =' '*indent+' <version ClassVersion="'+str(classVersion)+'" checksum="'+str(checksum)+'"/>\n'
if hasNoSubElements:
out += newLine
newLine=' '*indent+'</class>\n'
out +=newLine

outFile.writelines(out)

def main(args):
ClassesDefUtils.initROOT(args.library)
if args.library is None and args.checkdict:
print ("Dictionary checks require a specific library")
ClassesDefUtils.initCheckClass()

try:
p = ClassesDefUtils.XmlParser(args.xmlfile)
except RuntimeError as e:
print(f"Parsing {args.xmlfile} failed: {e}")
return(1)

(missingDict, foundErrors) = checkClassDefinitions(p.classes, args.checkdict)
(foundRootDoesNotMatchError, originalToNormalizedNames) = checkErrors(foundErrors, p.classes, args.generate)

if (len(foundErrors)>0 and not args.generate) or (args.generate and foundRootDoesNotMatchError) or missingDict:
return 1

if args.generate:
generate(args.xmlfile, 'classes_def.xml.generated', originalToNormalizedNames, p.classes, foundErrors)

return 0

if __name__ == "__main__":
from argparse import ArgumentParser, ArgumentDefaultsHelpFormatter
parser = ArgumentParser(formatter_class=ArgumentDefaultsHelpFormatter)
parser.add_argument("-d","--check_dictionaries", dest="checkdict",action="store_true",default=False,
help="check that all required dictionaries are loaded")
parser.add_argument("-l","--lib", dest="library", type=str,
help="specify the library to load. If not set classes are found using the PluginManager")
parser.add_argument("-x","--xml_file", dest="xmlfile",default="./classes_def.xml", type=str,
help="the classes_def.xml file to read")
parser.add_argument("-g","--generate_new",dest="generate", action="store_true",default=False,
help="instead of issuing errors, generate a new classes_def.xml file.")

args = parser.parse_args()
sys.exit(main(args))

1 change: 1 addition & 0 deletions FWCore/Reflection/test/BuildFile.xml
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@

<test name="TestFWCoreReflectionCheckClassVersion" command="run_checkClassVersion.sh"/>
<test name="TestFWCoreReflectionDumpClassVersion" command="run_dumpClassVersion.sh"/>
<test name="TestFWCoreReflectionClassVersionUpdate" command="run_classVersionUpdate.sh"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smuzaffar Another question on test dependencies. Is it possible for a test to declare a dependence on a library that is defined in the same BuildFile.xml (i.e. FWCoreReflectionTestObjects in this case)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@makortel , yes by adding <lib name="FWCoreReflectionTestObjects"/> in the <test .... > ` block

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah wait, this works for <bin .../> but let me check if it works for <test...../> too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, currently <test ..../> does not treat <lib name="SomeLocalLib"/> properly but it can be easily fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

10 changes: 10 additions & 0 deletions FWCore/Reflection/test/dumpClassVersion_reference_afterUpdate.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"edm::Wrapper<edmtest::reflection::IntObject>": {
"checksum": 536952283,
"version": 4
},
"edmtest::reflection::IntObject": {
"checksum": 2954816125,
"version": 3
}
}
76 changes: 76 additions & 0 deletions FWCore/Reflection/test/run_classVersionUpdate.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#!/bin/bash -e

SCRAM_TEST_NAME=TestFWCoreReflectionClassVersionUpdate
rm -rf $SCRAM_TEST_NAME
mkdir $SCRAM_TEST_NAME
cd $SCRAM_TEST_NAME

# Create a new CMSSW dev area and build modified DataFormats/TestObjects in it
NEW_CMSSW_BASE=$(/bin/pwd -P)/$CMSSW_VERSION
scram -a $SCRAM_ARCH project $CMSSW_VERSION
pushd $CMSSW_VERSION/src

# Copy FWCore/Reflection code to be able to edit it to make ROOT header parsing to fail
for DIR in ${CMSSW_BASE} ${CMSSW_RELEASE_BASE} ${CMSSW_FULL_RELEASE_BASE} ; do
if [ -d ${DIR}/src/FWCore/Reflection ]; then
mkdir FWCore
cp -Lr ${DIR}/src/FWCore/Reflection FWCore/
break
fi
done
if [ ! -e FWCore/Reflection ]; then
echo "Failed to symlink FWCore/Reflection from local or release area"
exit 1;
fi

# The original src/ tree is protected from writes in PR tests
chmod -R u+w FWCore/Reflection/test/stubs

# Modify the IntObject class to trigger a new version
#
# Just setting USER_CXXFLAGS for scram is not sufficient,because
# somehow ROOT (as used by edmCheckClassVersion) still picks up the
# version 3 of the class
echo "#define FWCORE_REFLECTION_TEST_INTOBJECT_V4" | cat - FWCore/Reflection/test/stubs/TestObjects.h > TestObjects.h.tmp
mv TestObjects.h.tmp FWCore/Reflection/test/stubs/TestObjects.h


#Set env and build in sub-shel
(eval $(scram run -sh) ; scram build -j $(nproc))

popd

# Prepend NEW_CMSSW_BASE's lib/src paths in to LD_LIBRARY_PATH and ROOT_INCLUDE_PATH
export LD_LIBRARY_PATH=${NEW_CMSSW_BASE}/lib/${SCRAM_ARCH}:${LD_LIBRARY_PATH}
export ROOT_INCLUDE_PATH=${NEW_CMSSW_BASE}/src:${ROOT_INCLUDE_PATH}

# Make the actual test
echo "Initial setup complete, now for the actual test"
XMLPATH=${SCRAM_TEST_PATH}/stubs
LIBFILE=libFWCoreReflectionTestObjects.so

function die { echo Failure $1: status $2 ; exit $2 ; }
function runFailure {
$1 -l ${LIBFILE} -x ${XMLPATH}/$2 > log.txt && die "$1 for $2 did not fail" 1
grep -q "$3" log.txt
RET=$?
if [ "$RET" != "0" ]; then
echo "$1 for $2 did not contain '$3', log is below"
cat log.txt
exit 1
fi
}

echo "edmCheckClassVersion tests"

runFailure edmCheckClassVersion classes_def.xml "error: class 'edmtest::reflection::IntObject' has a different checksum for ClassVersion 3. Increment ClassVersion to 4 and assign it to checksum 2954816125"
runFailure edmCheckClassVersion test_def_v4.xml "error: for class 'edmtest::reflection::IntObject' ROOT says the ClassVersion is 3 but classes_def.xml says it is 4. Are you sure everything compiled correctly?"

edmCheckClassVersion -l ${LIBFILE} -x ${XMLPATH}/classes_def.xml -g || die "edmCheckClassVersion -g failed" $?
diff -u ${XMLPATH}/test_def_v4.xml classes_def.xml.generated || die "classes_def.xml.generated differs from expectation" $?


echo "edmDumpClassVersion tests"

edmDumpClassVersion -l ${LIBFILE} -x ${XMLPATH}/classes_def.xml -o dump.json
diff -u ${SCRAM_TEST_PATH}/dumpClassVersion_reference_afterUpdate.json dump.json || die "Unexpected class version dump" $?
9 changes: 9 additions & 0 deletions FWCore/Reflection/test/stubs/TestObjects.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,19 @@ namespace edmtest::reflection {
IntObject();
IntObject(int v) : value_(v) {}

#ifdef FWCORE_REFLECTION_TEST_INTOBJECT_V4
void set(int v) {
value_ = v;
set_ = true;
}
#endif
int get() const { return value_; }

private:
int value_ = 0;
#ifdef FWCORE_REFLECTION_TEST_INTOBJECT_V4
bool set_ = false;
#endif
};
} // namespace edmtest::reflection

Expand Down
7 changes: 7 additions & 0 deletions FWCore/Reflection/test/stubs/test_def_v4.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<lcgdict>
<class name="edmtest::reflection::IntObject" ClassVersion="4">
<version ClassVersion="4" checksum="2954816125"/>
<version ClassVersion="3" checksum="427917710"/>
</class>
<class name="edm::Wrapper<edmtest::reflection::IntObject>"/>
</lcgdict>