[PATCH 1 of 8] bdiff: add _version to help detect breaking binary changes

[Gregory Szorc]

On Tue, May 2, 2017 at 5:19 PM, Jun Wu <quark at fb.com> wrote:

> # HG changeset patch
> # User Jun Wu <quark at fb.com>
> # Date 1493166881 25200
> #      Tue Apr 25 17:34:41 2017 -0700
> # Node ID 9073b1dfb58eb71c1005dad5267b6ee4f09790da
> # Parent  6cacc271ee0a9385be483314dc73be176a13c891
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r
> 9073b1dfb58e
> bdiff: add _version to help detect breaking binary changes
>
> Previously, we have no way to detect if a compiled .so file could be used
> or
> not, and blindly load it if it exists. Usually we carefully maintain
> compatibility of .so and fallback to pure code gracefully. But if we stick
> to the rules, certain nice changes will be impossible to make in a clean
> way.
>
> This patch adds a "_version" variable to the module so we can detect
> inconsistency and take appropriate actions (warn, abort, fallback to pure,
> run make automatically) in the module loader.
>

I like the spirit of this series.

Have you given any consideration to:

* Sharing a version number across all modules? That will make the code
easier at the expense of recompiling all C modules when not strictly
necessary. Since the base rate of changing C modules is low and everything
but zstd compiles almost instantaneously, one can make an argument that
simplicity is sufficient.

* A shared function or macro for calling PyModule_AddIntConstant(). It
feels like a bit of redundant code across all the modules.


>
> diff --git a/mercurial/bdiff_module.c b/mercurial/bdiff_module.c
> --- a/mercurial/bdiff_module.c
> +++ b/mercurial/bdiff_module.c
> @@ -193,4 +193,8 @@ static PyMethodDef methods[] = {
>  };
>
> +/* Binary version. When breaking changes are made, bump this number and
> change
> + * __init__.py, so the module loader can notice and fallback to pure. */
> +static const int _version = 1;
> +
>  #ifdef IS_PY3K
>  static struct PyModuleDef bdiff_module = {
> @@ -204,10 +208,15 @@ static struct PyModuleDef bdiff_module =
>  PyMODINIT_FUNC PyInit_bdiff(void)
>  {
> -       return PyModule_Create(&bdiff_module);
> +       PyObject *m;
> +       m = PyModule_Create(&bdiff_module);
> +       PyModule_AddIntConstant(m, "_version", _version);
> +       return m;
>  }
>  #else
>  PyMODINIT_FUNC initbdiff(void)
>  {
> -       Py_InitModule3("bdiff", methods, mdiff_doc);
> +       PyObject *m;
> +       m = Py_InitModule3("bdiff", methods, mdiff_doc);
> +       PyModule_AddIntConstant(m, "_version", _version);
>  }
>  #endif
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170502/72cf1921/attachment.html>