From da9e5a1b5681135cf6c304233b066a3f87e76459 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Tue, 30 Jul 2024 17:34:35 +0200 Subject: [PATCH 1/2] Add more tests for edmCheckClassVersion, and fix an a problem with -g option --- .../Reflection/scripts/edmCheckClassVersion | 2 +- FWCore/Reflection/test/BuildFile.xml | 1 + ...umpClassVersion_reference_afterUpdate.json | 10 +++ .../Reflection/test/run_classVersionUpdate.sh | 76 +++++++++++++++++++ FWCore/Reflection/test/stubs/TestObjects.h | 9 +++ FWCore/Reflection/test/stubs/test_def_v4.xml | 7 ++ 6 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 FWCore/Reflection/test/dumpClassVersion_reference_afterUpdate.json create mode 100755 FWCore/Reflection/test/run_classVersionUpdate.sh create mode 100644 FWCore/Reflection/test/stubs/test_def_v4.xml diff --git a/FWCore/Reflection/scripts/edmCheckClassVersion b/FWCore/Reflection/scripts/edmCheckClassVersion index 88fbcb58ba1f1..33abed4efdd12 100755 --- a/FWCore/Reflection/scripts/edmCheckClassVersion +++ b/FWCore/Reflection/scripts/edmCheckClassVersion @@ -113,7 +113,7 @@ if options.generate and not foundRootDoesNotMatchError and not missingDict: if normName is not None: indent = l.find('<') #this is a class with a problem - classVersion = p.classes[normName][XmlParser.classVersionIndex] + classVersion = p.classes[normName][ClassesDefUtils.XmlParser.classVersionIndex] code,checksum,rootClassVersion = foundErrors[normName] hasNoSubElements = (-1 != l.find('/>')) if code == ClassesDefUtils.errorMustUpdateClassVersion: diff --git a/FWCore/Reflection/test/BuildFile.xml b/FWCore/Reflection/test/BuildFile.xml index 10e0d1ae71daf..c110297ef919f 100644 --- a/FWCore/Reflection/test/BuildFile.xml +++ b/FWCore/Reflection/test/BuildFile.xml @@ -6,3 +6,4 @@ + diff --git a/FWCore/Reflection/test/dumpClassVersion_reference_afterUpdate.json b/FWCore/Reflection/test/dumpClassVersion_reference_afterUpdate.json new file mode 100644 index 0000000000000..d9323142d5ee9 --- /dev/null +++ b/FWCore/Reflection/test/dumpClassVersion_reference_afterUpdate.json @@ -0,0 +1,10 @@ +{ + "edm::Wrapper": { + "checksum": 536952283, + "version": 4 + }, + "edmtest::reflection::IntObject": { + "checksum": 2954816125, + "version": 3 + } +} \ No newline at end of file diff --git a/FWCore/Reflection/test/run_classVersionUpdate.sh b/FWCore/Reflection/test/run_classVersionUpdate.sh new file mode 100755 index 0000000000000..261d573294ae5 --- /dev/null +++ b/FWCore/Reflection/test/run_classVersionUpdate.sh @@ -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" $? diff --git a/FWCore/Reflection/test/stubs/TestObjects.h b/FWCore/Reflection/test/stubs/TestObjects.h index 0ebbb9ec79fb4..cdfec7c443260 100644 --- a/FWCore/Reflection/test/stubs/TestObjects.h +++ b/FWCore/Reflection/test/stubs/TestObjects.h @@ -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 diff --git a/FWCore/Reflection/test/stubs/test_def_v4.xml b/FWCore/Reflection/test/stubs/test_def_v4.xml new file mode 100644 index 0000000000000..f606f0da81579 --- /dev/null +++ b/FWCore/Reflection/test/stubs/test_def_v4.xml @@ -0,0 +1,7 @@ + + + + + + + From a2d77f23c084d3cff3e8cabb62894de41c2fc9d5 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Wed, 31 Jul 2024 18:39:10 +0200 Subject: [PATCH 2/2] Refactor edmCheckClassVersion --- .../Reflection/scripts/edmCheckClassVersion | 201 +++++++++--------- 1 file changed, 106 insertions(+), 95 deletions(-) diff --git a/FWCore/Reflection/scripts/edmCheckClassVersion b/FWCore/Reflection/scripts/edmCheckClassVersion index 33abed4efdd12..ecd795cabbbac 100755 --- a/FWCore/Reflection/scripts/edmCheckClassVersion +++ b/FWCore/Reflection/scripts/edmCheckClassVersion @@ -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 (' ') - - -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('')) - 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+' \n' - if hasNoSubElements: - out += newLine - newLine=' '*indent+'\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 (' ') + 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('')) + 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+' \n' + if hasNoSubElements: + out += newLine + newLine=' '*indent+'\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))