From f596b11b6bef7c0c65fefd8a76d1feca643750b8 Mon Sep 17 00:00:00 2001 From: Peter Shinners Date: Mon, 24 May 2021 17:53:09 -0700 Subject: [PATCH 1/3] Handle Python 3 iterating over modified dictionaries Under Python 3, the dictionary values() call gives a view object that raises RuntimeError if the dictionary is modified while iterating. Technically that also happened under Python 2 if the dictionary itervalues() method was used. This uses a simple _copiedvalues() function to get a standalone list of values efficiently on either version of Python. The recursion functions for find, save/write, and close use this to prevent exceptions. The behavior under Python 2 is unchanged. --- cask.py | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/cask.py b/cask.py index d312eee..879629f 100644 --- a/cask.py +++ b/cask.py @@ -47,6 +47,7 @@ import os import re +import sys import imath import ctypes import weakref @@ -412,7 +413,7 @@ def find_iter(obj, name=".*", types=None): """ if re.match(name, obj.name) and (types is None or obj.type() in types): yield obj - for child in obj.children.values(): + for child in _copiedvalues(obj.children): for grandchild in find_iter(child, name, types): yield grandchild @@ -719,13 +720,13 @@ def close(self): """Closes this archive and makes it immutable.""" def close_tree(obj): """recursive close""" - for child in obj.children.values(): + for child in _copiedvalues(obj.children): close_tree(child) del child obj.close() del obj - for child in self.top.children.values(): + for child in _copiedvalues(self.top.children): close_tree(child) del child @@ -747,13 +748,13 @@ def __write(self): def save_tree(obj): """recursive save""" obj.save() - for child in obj.children.values(): + for child in _copiedvalues(obj.children): save_tree(child) child.close() del child obj.close() del obj - for child in self.top.children.values(): + for child in _copiedvalues(self.top.children): save_tree(child) self.top.close() @@ -1139,7 +1140,7 @@ def close(self): self._klass = None self._parent = None self._values = [] - for prop in self.properties.values(): + for prop in _copiedvalues(self.properties): prop.close() def save(self): @@ -1158,7 +1159,7 @@ def save(self): % (self.name, value, self._klass, err)) del value else: - for prop in self.properties.values(): + for prop in _copiedvalues(self.properties): up = False if not prop.iobject and not prop.object().iobject: if prop.name == ".childBnds": @@ -1537,7 +1538,7 @@ def close(self): self._parent = None self._schema = None self.clear_all() - for prop in self.properties.values(): + for prop in _copiedvalues(self.properties): prop.close() del prop @@ -1545,7 +1546,7 @@ def save(self): """Walks child and property sub-trees creating OObjects as necessary. """ obj = self.oobject - for prop in self.properties.values(): + for prop in _copiedvalues(self.properties): prop.save() prop.close() del prop @@ -1689,3 +1690,13 @@ class Points(Object): _sample_class = alembic.AbcGeom.OPointsSchemaSample def __init__(self, *args, **kwargs): super(Points, self).__init__(*args, **kwargs) + + +if sys.version[0] == "2": + def _copiedvalues(d): + """A copied list of dictionary values. (Python 2)""" + return d.values() +else: + def _copiedvalues(d): + """A copied list of dictionary values. (Python 3)""" + return list(d.values()) From 346349fe867c17f4b7ce589d8054df074f91260b Mon Sep 17 00:00:00 2001 From: Peter Shinners Date: Tue, 25 May 2021 11:29:09 -0700 Subject: [PATCH 2/3] Unit tests work with Python 3 This requires no changes to the cask library itself. But the testing needed a few changes to work on Python 3. This also updates the tests to pass on Windows. --- test/testCask.py | 66 ++++++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/test/testCask.py b/test/testCask.py index db12445..b0d0c35 100644 --- a/test/testCask.py +++ b/test/testCask.py @@ -36,7 +36,6 @@ import os import sys -import string import unittest import tempfile @@ -65,10 +64,10 @@ def set_float(schema, target, shaderType, paramName, value): def lights_out(): filename = os.path.join(TEMPDIR, "cask_test_lights.abc") - if os.path.exists(filename) and cask.is_valid(filename): - return filename - - os.system("rm -f %s" % filename) + if os.path.exists(filename): + if cask.is_valid(filename): + return filename + os.unlink(filename) archive = alembic.Abc.OArchive(filename) lightA = alembic.AbcGeom.OLight(archive.getTop(), "lightA") lightB = alembic.AbcGeom.OLight(archive.getTop(), "lightB") @@ -168,12 +167,15 @@ def deep_out(): if os.path.exists(filename) and cask.is_valid(filename): return filename + uppers = "ABCDEFGHIJKLMNOP" + lowers = uppers.lower() + obj = alembic.Abc.OArchive(filename).getTop() for i in range(10): - obj = alembic.AbcGeom.OXform(obj, string.uppercase[i]) + obj = alembic.AbcGeom.OXform(obj, uppers[i]) p = obj.getProperties() for i in range(3): - p = alembic.Abc.OCompoundProperty(p, string.lowercase[i]) + p = alembic.Abc.OCompoundProperty(p, lowers[i]) p = alembic.Abc.OStringProperty(p, "myprop") p.setValue("foo") @@ -190,7 +192,7 @@ def test_write_basic(self): # create xform object named foo and make it a child of top f = a.top.children["foo"] = cask.Xform() self.assertEqual(len(a.top.children), 1) - self.assertEqual(a.top.children.values()[0].name, "foo") + self.assertEqual(_dictvalue(a.top.children).name, "foo") # create some simple properties b = f.properties["bar"] = cask.Property() @@ -430,18 +432,18 @@ def test_rename(self): # create a new object and property t.children["foo"] = cask.Xform() t.properties["some"] = cask.Property() - f = t.children.values()[0] - p = t.properties.values()[0] + f = _dictvalue(t.children) + p = _dictvalue(t.properties) self.assertEqual(f.name, "foo") self.assertEqual(p.name, "some") # rename them f.name = "bar" self.assertEqual(f.name, "bar") - self.assertEqual(t.children.values()[0].name, "bar") + self.assertEqual(_dictvalue(t.children).name, "bar") p.name = "thing" self.assertEqual(p.name, "thing") - self.assertEqual(t.properties.values()[0].name, "thing") + self.assertEqual(_dictvalue(t.properties).name, "thing") # test for accessor updates self.assertEqual(t.children["bar"], f) @@ -477,7 +479,7 @@ def test_reassign(self): p = t.properties["prop"] p.name = "new" self.assertRaises(KeyError, t.properties.__getitem__, "prop") - self.assertEqual(t.properties.values()[0].name, "new") + self.assertEqual(_dictvalue(t.properties).name, "new") self.assertEqual(t.properties["new"], p) # another rename test @@ -556,7 +558,7 @@ def test_deep_dict(self): self.assertRaises(KeyError, t.children.__getitem__, "A/B/C/Z") # property accessors - x = t.children.values()[0] + x = _dictvalue(t.children) self.assertEqual(x.name, "A") self.assertEqual(x.properties["a/b/c/myprop"].values[0], "foo") @@ -819,12 +821,13 @@ def test_verify_write_basic(self): a = cask.Archive(filename) self.assertEqual(len(a.top.children), 1) - self.assertEqual(a.top.children.values()[0].name, "foo") - self.assertEqual(type(a.top.children.values()[0]), cask.Xform) - self.assertEqual(len(a.top.children.values()[0].properties), 3) + child = _dictvalue(a.top.children) + self.assertEqual(child.name, "foo") + self.assertEqual(type(child), cask.Xform) + self.assertEqual(len(child.properties), 3) - self.assertEqual(a.top.children.values()[0].properties["bar"].get_value(), "hello") - self.assertEqual(a.top.children.values()[0].properties["baz"].get_value(), 42.0) + self.assertEqual(child.properties["bar"].get_value(), "hello") + self.assertEqual(child.properties["baz"].get_value(), 42.0) def test_read_lights(self): filepath = lights_out() @@ -953,7 +956,7 @@ def test_verify_copy_mesh(self): # open the archive a = cask.Archive(filename) - p = a.top.children.values()[0] + p = _dictvalue(a.top.children) # verify timesamplings were copied self.assertEqual(len(a.timesamplings), 1) @@ -996,8 +999,8 @@ def test_verify_write_mesh(self): # get the objects a = cask.Archive(filename) - x = a.top.children.values()[0] - p = x.children.values()[0] + x = _dictvalue(a.top.children) + p = _dictvalue(x.children) # verify the hierarchy self.assertEqual(x.name, "foo") @@ -1006,7 +1009,7 @@ def test_verify_write_mesh(self): self.assertEqual(type(p), cask.PolyMesh) # check one of the properties - vals = x.properties.values()[0].properties[".vals"] + vals = _dictvalue(x.properties).properties[".vals"] self.assertEqual(vals.values[0], imath.V3d(1, 2, 3)) def test_verify_write_points(self): @@ -1015,8 +1018,8 @@ def test_verify_write_points(self): # get the objects a = cask.Archive(filename) - x = a.top.children.values()[0] - pts = x.children.values()[0] + x = _dictvalue(a.top.children) + pts = _dictvalue(x.children) # verify the hierarchy self.assertEqual(x.name, "points") @@ -1058,8 +1061,8 @@ def test_verify_insert_node(self): # get some objects a = cask.Archive(filename) - r = a.top.children.values()[0] - m = r.children.values()[0] + r = _dictvalue(a.top.children) + m = _dictvalue(r.children) # verify re-parenting self.assertEqual(r.name, "root") @@ -1179,7 +1182,7 @@ def test_issue_318(self): # access an object in the file, then close it a = cask.Archive(test_file_1) - self.assertEqual(a.top.children.keys(), ['meshy']) + self.assertEqual(list(a.top.children), ['meshy']) a.close() # try to write to the same file path @@ -1362,8 +1365,8 @@ def test_issue_23(self): # read it back in and check for the user properties a = cask.Archive(test_file) x = a.top.children["x"] - self.assertEqual(x.properties.keys(), [".xform"]) - self.assertEqual(x.properties[".xform"].properties.keys(), + self.assertEqual(list(x.properties), [".xform"]) + self.assertEqual(list(x.properties[".xform"].properties), [".userProperties"]) up = x.properties[".xform/.userProperties"] @@ -1456,5 +1459,8 @@ def test_issue_8(self): a1.close() +def _dictvalue(d): + return next(iter(d.values())) + if __name__ == '__main__': unittest.main() From 783c9c31190c7bd64a76a60343f1e888fa4531f7 Mon Sep 17 00:00:00 2001 From: Peter Shinners Date: Sun, 29 Jan 2023 13:43:47 -0700 Subject: [PATCH 3/3] Cleaner python 3 dictionary values The cleanest way for dictionary.values() to match the Python 2 behavior is to override the DeepDict.values() method directly. --- cask.py | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/cask.py b/cask.py index 879629f..dfef6c8 100644 --- a/cask.py +++ b/cask.py @@ -413,7 +413,7 @@ def find_iter(obj, name=".*", types=None): """ if re.match(name, obj.name) and (types is None or obj.type() in types): yield obj - for child in _copiedvalues(obj.children): + for child in obj.children.values(): for grandchild in find_iter(child, name, types): yield grandchild @@ -512,6 +512,16 @@ def remove(self, key): if key and key in self: self.pop(key) + if sys.version[0] != "2": + # Code in this module was written in the Python 2 style, where + # a dictionary's `values()` is an independent copy of all the data. + # Since Python 3, `values()` becomes a view, which raises + # `RuntimeError` if the dictionary is modified while iterating. + def values(self): + """Dictionary values copied into a list, to preserve Python 2 behavior""" + view = super(DeepDict, self).values() + return list(view) + class Archive(object): """Archive I/O Object""" @@ -720,13 +730,13 @@ def close(self): """Closes this archive and makes it immutable.""" def close_tree(obj): """recursive close""" - for child in _copiedvalues(obj.children): + for child in obj.children.values(): close_tree(child) del child obj.close() del obj - for child in _copiedvalues(self.top.children): + for child in self.top.children.values(): close_tree(child) del child @@ -748,13 +758,13 @@ def __write(self): def save_tree(obj): """recursive save""" obj.save() - for child in _copiedvalues(obj.children): + for child in obj.children.values(): save_tree(child) child.close() del child obj.close() del obj - for child in _copiedvalues(self.top.children): + for child in self.top.children.values(): save_tree(child) self.top.close() @@ -1140,7 +1150,7 @@ def close(self): self._klass = None self._parent = None self._values = [] - for prop in _copiedvalues(self.properties): + for prop in self.properties.values(): prop.close() def save(self): @@ -1159,7 +1169,7 @@ def save(self): % (self.name, value, self._klass, err)) del value else: - for prop in _copiedvalues(self.properties): + for prop in self.properties.values(): up = False if not prop.iobject and not prop.object().iobject: if prop.name == ".childBnds": @@ -1538,7 +1548,7 @@ def close(self): self._parent = None self._schema = None self.clear_all() - for prop in _copiedvalues(self.properties): + for prop in self.properties.values(): prop.close() del prop @@ -1546,7 +1556,7 @@ def save(self): """Walks child and property sub-trees creating OObjects as necessary. """ obj = self.oobject - for prop in _copiedvalues(self.properties): + for prop in self.properties.values(): prop.save() prop.close() del prop @@ -1690,13 +1700,3 @@ class Points(Object): _sample_class = alembic.AbcGeom.OPointsSchemaSample def __init__(self, *args, **kwargs): super(Points, self).__init__(*args, **kwargs) - - -if sys.version[0] == "2": - def _copiedvalues(d): - """A copied list of dictionary values. (Python 2)""" - return d.values() -else: - def _copiedvalues(d): - """A copied list of dictionary values. (Python 3)""" - return list(d.values())