From 3fafd46da630868ab385b7b8c2776bacf83301b8 Mon Sep 17 00:00:00 2001 From: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com> Date: Fri, 29 Mar 2024 19:26:25 +0100 Subject: [PATCH] Document how to handle compatibility breakages --- about/release_policy.rst | 3 +- .../handling_compatibility_breakages.rst | 141 ++++++++++++++++++ contributing/development/index.rst | 1 + 3 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 contributing/development/handling_compatibility_breakages.rst diff --git a/about/release_policy.rst b/about/release_policy.rst index 31a85dbb0a8..734a34b4381 100644 --- a/about/release_policy.rst +++ b/about/release_policy.rst @@ -275,5 +275,4 @@ compatibility-breaking changes of this kind. optional parameter), a GDExtension compatibility method must be created. This ensures that existing GDExtensions continue to work across patch and minor releases, so that users don't have to recompile them. - See `pull request #76446 `_ - for more information. + See :ref:`doc_handling_compatibility_breakages` for more information. diff --git a/contributing/development/handling_compatibility_breakages.rst b/contributing/development/handling_compatibility_breakages.rst new file mode 100644 index 00000000000..3c2ed2e259f --- /dev/null +++ b/contributing/development/handling_compatibility_breakages.rst @@ -0,0 +1,141 @@ +.. _doc_handling_compatibility_breakages: + +Handling compatibility breakages +================================ + +.. TODO: Elaborate on types of compatibility and procedure. + +So you've added a new parameter to a method, changed the return type, +changed the type of a parameter, or changed its default value, +and now the automated testing is complaining about compatibility breakages? + +Breaking compatibility should be avoided, but when necessary there are systems in place +to handle this in a way that makes the transition as smooth as possible. + +A practical example +------------------- + +.. TODO: Add example that showcases more details like original default arguments etc. + +These changes are taken from `pull request #88047 `_, which added +new pathing options to ``AStarGrid2D`` and other AStar classes. +Among other changes, these methods were modified in ``core/math/a_star_grid_2d.h``: + +.. code-block:: cpp + + Vector get_point_path(const Vector2i &p_from, const Vector2i &p_to); + TypedArray get_id_path(const Vector2i &p_from, const Vector2i &p_to); + +To: + +.. code-block:: cpp + + Vector get_point_path(const Vector2i &p_from, const Vector2i &p_to, bool p_allow_partial_path = false); + TypedArray get_id_path(const Vector2i &p_from, const Vector2i &p_to, bool p_allow_partial_path = false); + +This meant adding new compatibility method bindings to the file, which should be in the ``protected`` section of +the code, usually placed next to ``_bind_methods()``: + +.. code-block:: cpp + + #ifndef DISABLE_DEPRECATED + TypedArray _get_id_path_bind_compat_88047(const Vector2i &p_from, const Vector2i &p_to); + Vector _get_point_path_bind_compat_88047(const Vector2i &p_from, const Vector2i &p_to); + static void _bind_compatibility_methods(); + #endif + +They should start with a ``_`` to indicate that they are internal, and end with ``_bind_compat_`` followed by the PR number +that introduced the change (``88047`` in this example). These compatibility methods need to be implemented in a dedicated file, +like ``core/math/a_star_grid_2d.compat.inc`` in this case: + +.. code-block:: cpp + + /**************************************************************************/ + /* a_star_grid_2d.compat.inc */ + /**************************************************************************/ + /* This file is part of: */ + /* GODOT ENGINE */ + /* https://godotengine.org */ + /**************************************************************************/ + /* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */ + /* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */ + /* */ + /* Permission is hereby granted, free of charge, to any person obtaining */ + /* a copy of this software and associated documentation files (the */ + /* "Software"), to deal in the Software without restriction, including */ + /* without limitation the rights to use, copy, modify, merge, publish, */ + /* distribute, sublicense, and/or sell copies of the Software, and to */ + /* permit persons to whom the Software is furnished to do so, subject to */ + /* the following conditions: */ + /* */ + /* The above copyright notice and this permission notice shall be */ + /* included in all copies or substantial portions of the Software. */ + /* */ + /* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */ + /* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */ + /* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */ + /* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */ + /* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */ + /* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */ + /* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ + /**************************************************************************/ + + #ifndef DISABLE_DEPRECATED + + #include "core/variant/typed_array.h" + + TypedArray AStarGrid2D::_get_id_path_bind_compat_88047(const Vector2i &p_from_id, const Vector2i &p_to_id) { + return get_id_path(p_from_id, p_to_id, false); + } + + Vector AStarGrid2D::_get_point_path_bind_compat_88047(const Vector2i &p_from_id, const Vector2i &p_to_id) { + return get_point_path(p_from_id, p_to_id, false); + } + + void AStarGrid2D::_bind_compatibility_methods() { + ClassDB::bind_compatibility_method(D_METHOD("get_id_path", "from_id", "to_id"), &AStarGrid2D::_get_id_path_bind_compat_88047); + ClassDB::bind_compatibility_method(D_METHOD("get_point_path", "from_id", "to_id"), &AStarGrid2D::_get_point_path_bind_compat_88047); + } + + #endif // DISABLE_DEPRECATED + +Unless the change in compatibility is complex, the compatibility method should simply call the modified method directly, +instead of duplicating that method. Make sure to match the default arguments for that method (in the example above this would be ``false``). + +This file should always be placed next to the original file, and have ``.compat.inc`` at the end instead of ``.cpp`` or ``.h``. +Next, this should be included in the ``.cpp`` file we're adding compatibility methods to, so ``core/math/a_star_grid_2d.cpp``: + +.. code-block:: cpp + + #include "a_star_grid_2d.h" + #include "a_star_grid_2d.compat.inc" + + #include "core/variant/typed_array.h" + +And finally, the changes reported by the API validation step should be added to the relevant validation file. Because this was +done during the development of 4.3, this would be ``misc/extension_api_validation/4.2-stable.expected`` (including changes not shown in +this example): + +.. code-block:: text + + GH-88047 + -------- + Validate extension JSON: Error: Field 'classes/AStar2D/methods/get_id_path/arguments': size changed value in new API, from 2 to 3. + Validate extension JSON: Error: Field 'classes/AStar2D/methods/get_point_path/arguments': size changed value in new API, from 2 to 3. + Validate extension JSON: Error: Field 'classes/AStar3D/methods/get_id_path/arguments': size changed value in new API, from 2 to 3. + Validate extension JSON: Error: Field 'classes/AStar3D/methods/get_point_path/arguments': size changed value in new API, from 2 to 3. + Validate extension JSON: Error: Field 'classes/AStarGrid2D/methods/get_id_path/arguments': size changed value in new API, from 2 to 3. + Validate extension JSON: Error: Field 'classes/AStarGrid2D/methods/get_point_path/arguments': size changed value in new API, from 2 to 3. + + Added optional "allow_partial_path" argument to get_id_path and get_point_path methods in AStar classes. + Compatibility methods registered. + +The instructions for how to add to that file are at the top of the file itself. + +If you get a "Hash changed" error for a method, it means that the compatibility binding is missing or incorrect. +Such lines shouldn't be added to the ``.expected`` file, but fixed by binding the proper compatibility method. + +And that's it! You might run into a bit more complicated cases, like rearranging arguments, +changing return types, etc., but this covers the basic on how to use this system. + +For more information, see `pull request #76446 `_. diff --git a/contributing/development/index.rst b/contributing/development/index.rst index a5a0cfe5267..ce3a3f746ae 100644 --- a/contributing/development/index.rst +++ b/contributing/development/index.rst @@ -19,6 +19,7 @@ especially if you're new to using Git or GitHub. best_practices_for_engine_contributors code_style_guidelines cpp_usage_guidelines + handling_compatibility_breakages Buildsystem and work environment --------------------------------