From a45fbb84da31222f8d0d712df9dd0ca672e4c058 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 8 Feb 2023 13:16:29 +0100 Subject: [PATCH] python: handle Windows path insanities in the bindings directly. Otherwise people wouldn't be able to conveniently use os.path, which is an undesirable pain point. And do this for all currently exposed APIs, not just the AbstractImporter that caused a problem. This reverts commit 28497ec2ca4990ad37d0b9d6263cf83855ab87b8. --- doc/python/magnum.text.rst | 5 +++ doc/python/magnum.trade.rst | 15 +++++++ src/python/magnum/test/test_trade.py | 60 ++++++++++------------------ src/python/magnum/text.cpp | 18 ++++++++- src/python/magnum/trade.cpp | 40 +++++++++++++++++-- 5 files changed, 94 insertions(+), 44 deletions(-) diff --git a/doc/python/magnum.text.rst b/doc/python/magnum.text.rst index 5488806..5a9ea59 100644 --- a/doc/python/magnum.text.rst +++ b/doc/python/magnum.text.rst @@ -53,6 +53,11 @@ .. py:function:: magnum.text.AbstractFont.open_file :raise RuntimeError: If file opening fails + For compatibility with :ref:`os.path`, on Windows this function converts + all backslashes in :p:`filename` to forward slashes before passing it to + :dox:`Text::AbstractFont::openFile()`, which expects forward slashes as + directory separators on all platforms. + .. py:property:: magnum.text.AbstractFont.size :raise AssertionError: If no file is opened .. py:property:: magnum.text.AbstractFont.ascent diff --git a/doc/python/magnum.trade.rst b/doc/python/magnum.trade.rst index 425fccb..5d7bf5b 100644 --- a/doc/python/magnum.trade.rst +++ b/doc/python/magnum.trade.rst @@ -189,6 +189,11 @@ .. py:function:: magnum.trade.AbstractImporter.open_file :raise RuntimeError: If file opening fails + For compatibility with :ref:`os.path`, on Windows this function converts + all backslashes in :p:`filename` to forward slashes before passing it to + :dox:`Trade::AbstractImporter::openFile()`, which expects forward slashes + as directory separators on all platforms. + .. py:property:: magnum.trade.AbstractImporter.mesh_count :raise AssertionError: If no file is opened .. py:function:: magnum.trade.AbstractImporter.mesh_level_count @@ -297,6 +302,11 @@ .. py:function:: magnum.trade.AbstractImageConverter.convert_to_file :raise RuntimeError: If image conversion fails + For compatibility with :ref:`os.path`, on Windows this function converts + all backslashes in :p:`filename` to forward slashes before passing it to + :dox:`Trade::AbstractImageConverter::convertToFile()`, which expects + forward slashes as directory separators on all platforms. + .. py:class:: magnum.trade.SceneConverterManager :summary: Manager for :ref:`AbstractSceneConverter` plugin instances @@ -324,3 +334,8 @@ .. py:function:: magnum.trade.AbstractSceneConverter.convert_to_file :raise RuntimeError: If scene conversion fails + + For compatibility with :ref:`os.path`, on Windows this function converts + all backslashes in :p:`filename` to forward slashes before passing it to + :dox:`Trade::AbstractSceneConverter::convertToFile()`, which expects + forward slashes as directory separators on all platforms. diff --git a/src/python/magnum/test/test_trade.py b/src/python/magnum/test/test_trade.py index 3ad3089..19ef715 100644 --- a/src/python/magnum/test/test_trade.py +++ b/src/python/magnum/test/test_trade.py @@ -103,8 +103,7 @@ class ImageData(unittest.TestCase): class MeshData(unittest.TestCase): def test(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) self.assertEqual(mesh.primitive, MeshPrimitive.TRIANGLES) @@ -197,8 +196,7 @@ class MeshData(unittest.TestCase): def test_index_data_access(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) mesh_refcount = sys.getrefcount(mesh) @@ -221,8 +219,7 @@ class MeshData(unittest.TestCase): def test_vertex_data_access(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) mesh_refcount = sys.getrefcount(mesh) @@ -245,8 +242,7 @@ class MeshData(unittest.TestCase): def test_indices_access(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) mesh_refcount = sys.getrefcount(mesh) @@ -275,8 +271,7 @@ class MeshData(unittest.TestCase): def test_attribute_access(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) mesh_refcount = sys.getrefcount(mesh) @@ -336,8 +331,7 @@ class MeshData(unittest.TestCase): def test_mutable_index_data_access(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) self.assertEqual(mesh.index_data_flags, trade.DataFlag.OWNED|trade.DataFlag.MUTABLE) @@ -354,8 +348,7 @@ class MeshData(unittest.TestCase): def test_mutable_vertex_data_access(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) self.assertEqual(mesh.vertex_data_flags, trade.DataFlag.OWNED|trade.DataFlag.MUTABLE) @@ -372,8 +365,7 @@ class MeshData(unittest.TestCase): def test_mutable_indices_access(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) self.assertEqual(mesh.index_data_flags, trade.DataFlag.OWNED|trade.DataFlag.MUTABLE) @@ -388,8 +380,7 @@ class MeshData(unittest.TestCase): def test_mutable_attributes_access(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) self.assertEqual(mesh.index_data_flags, trade.DataFlag.OWNED|trade.DataFlag.MUTABLE) @@ -431,8 +422,7 @@ class MeshData(unittest.TestCase): def test_nonindexed(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(1) self.assertFalse(mesh.is_indexed) @@ -456,8 +446,7 @@ class MeshData(unittest.TestCase): def test_attribute_oob(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) @@ -513,8 +502,7 @@ class MeshData(unittest.TestCase): def test_attribute_access_array(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) joint_ids_id = mesh.attribute_id(trade.MeshAttribute.JOINT_IDS) @@ -530,8 +518,7 @@ class MeshData(unittest.TestCase): def test_attribute_access_unsupported_format(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(0) custom_attribute_id = mesh.attribute_id(importer.mesh_attribute_for_name("_CUSTOM_ATTRIBUTE")) @@ -669,8 +656,7 @@ class Importer(unittest.TestCase): self.assertIsNone(importer.mesh_attribute_name(trade.MeshAttribute(32768 + 7))) self.assertIsNone(importer.mesh_attribute_for_name("_CUSTOM_ATTRIBUTE")) - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) self.assertEqual(importer.mesh_count, 2) self.assertEqual(importer.mesh_level_count(0), 1) self.assertEqual(importer.mesh_name(0), 'Indexed mesh') @@ -693,32 +679,28 @@ class Importer(unittest.TestCase): def test_mesh_level_oob(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) with self.assertRaises(IndexError): importer.mesh(0, 1) def test_mesh_by_name(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh('Non-indexed mesh') self.assertEqual(mesh.primitive, MeshPrimitive.TRIANGLES) def test_mesh_by_name_not_found(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) with self.assertRaises(KeyError): importer.mesh('Nonexistent') def test_mesh_by_name_level_oob(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) with self.assertRaises(IndexError): importer.mesh('Non-indexed mesh', 1) @@ -798,8 +780,7 @@ class ImageConverter(unittest.TestCase): class SceneConverter(unittest.TestCase): def test_mesh(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(1) converter = trade.SceneConverterManager().load_and_instantiate('StanfordSceneConverter') @@ -810,8 +791,7 @@ class SceneConverter(unittest.TestCase): def test_mesh_failed(self): importer = trade.ImporterManager().load_and_instantiate('GltfImporter') - # TODO figure out a less silly way to get forward slashes on Windows - importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf').replace('\\', '/')) + importer.open_file(os.path.join(os.path.dirname(__file__), 'mesh.gltf')) mesh = importer.mesh(1) converter = trade.SceneConverterManager().load_and_instantiate('AnySceneConverter') diff --git a/src/python/magnum/text.cpp b/src/python/magnum/text.cpp index 8d3b8a8..21ccebd 100644 --- a/src/python/magnum/text.cpp +++ b/src/python/magnum/text.cpp @@ -33,6 +33,13 @@ #include "corrade/pluginmanager.h" #include "magnum/bootstrap.h" +#ifdef CORRADE_TARGET_WINDOWS +/* To allow people to conveniently use Python's os.path, we need to convert + backslashes to forward slashes as all Corrade and Magnum APIs expect + forward */ +#include +#endif + namespace magnum { namespace { @@ -109,7 +116,16 @@ void text(py::module_& m) { .def("open_file", [](Text::AbstractFont& self, const std::string& filename, Float size) { /** @todo log redirection -- but we'd need assertions to not be part of that so when it dies, the user can still see why */ - if(self.openFile(filename, size)) return; + if(self.openFile( + #ifdef CORRADE_TARGET_WINDOWS + /* To allow people to conveniently use Python's os.path, we + need to convert backslashes to forward slashes as all + Corrade and Magnum APIs expect forward */ + Utility::Path::fromNativeSeparators(filename) + #else + filename + #endif + , size)) return; PyErr_Format(PyExc_RuntimeError, "opening %s failed", filename.data()); throw py::error_already_set{}; diff --git a/src/python/magnum/trade.cpp b/src/python/magnum/trade.cpp index 7d18f2e..7eb8779 100644 --- a/src/python/magnum/trade.cpp +++ b/src/python/magnum/trade.cpp @@ -46,6 +46,13 @@ #include "corrade/pluginmanager.h" #include "magnum/bootstrap.h" +#ifdef CORRADE_TARGET_WINDOWS +/* To allow people to conveniently use Python's os.path, we need to convert + backslashes to forward slashes as all Corrade and Magnum APIs expect + forward */ +#include +#endif + namespace magnum { namespace { @@ -282,7 +289,16 @@ template(Trade::AbstractImporter::*f)(UnsignedI template void checkImageConverterResult(Trade::AbstractImageConverter& self, const T& image, const std::string& filename) { /** @todo log redirection -- but we'd need assertions to not be part of that so when it dies, the user can still see why */ - bool out = (self.*f)(image, filename); + bool out = (self.*f)(image, + #ifdef CORRADE_TARGET_WINDOWS + /* To allow people to conveniently use Python's os.path, we need to + convert backslashes to forward slashes as all Corrade and Magnum + APIs expect forward */ + Utility::Path::fromNativeSeparators(filename) + #else + filename + #endif + ); if(!out) { PyErr_SetString(PyExc_RuntimeError, "conversion failed"); throw py::error_already_set{}; @@ -295,7 +311,16 @@ template void checkSceneConverterResult(Trade::AbstractSceneConverter& self, const T& mesh, const std::string& filename) { /** @todo log redirection -- but we'd need assertions to not be part of that so when it dies, the user can still see why */ - bool out = (self.*f)(mesh, filename); + bool out = (self.*f)(mesh, + #ifdef CORRADE_TARGET_WINDOWS + /* To allow people to conveniently use Python's os.path, we need to + convert backslashes to forward slashes as all Corrade and Magnum + APIs expect forward */ + Utility::Path::fromNativeSeparators(filename) + #else + filename + #endif + ); if(!out) { PyErr_SetString(PyExc_RuntimeError, "conversion failed"); throw py::error_already_set{}; @@ -691,7 +716,16 @@ void trade(py::module_& m) { .def("open_file", [](Trade::AbstractImporter& self, const std::string& filename) { /** @todo log redirection -- but we'd need assertions to not be part of that so when it dies, the user can still see why */ - if(self.openFile(filename)) return; + if(self.openFile( + #ifdef CORRADE_TARGET_WINDOWS + /* To allow people to conveniently use Python's os.path, we + need to convert backslashes to forward slashes as all + Corrade and Magnum APIs expect forward */ + Utility::Path::fromNativeSeparators(filename) + #else + filename + #endif + )) return; PyErr_Format(PyExc_RuntimeError, "opening %s failed", filename.data()); throw py::error_already_set{};