Merge pull request #45610 from makortel/edmCheckClassVersionRefactor

Refactor `edmCheckClassVersion`

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 ('   <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))
 

FWCore/Reflection/test/BuildFile.xml

@@ -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"/>

FWCore/Reflection/test/dumpClassVersion_reference_afterUpdate.json

@@ -0,0 +1,10 @@
+{
+ "edm::Wrapper<edmtest::reflection::IntObject>": {
+  "checksum": 536952283,
+  "version": 4
+ },
+ "edmtest::reflection::IntObject": {
+  "checksum": 2954816125,
+  "version": 3
+ }
+}

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" $?

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
 

FWCore/Reflection/test/stubs/test_def_v4.xml

@@ -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>