Patchwork [CREW] parsers: add apiversion module constant to C implementation of parsers

login
register
mail settings
Submitter Chris Jerdonek
Date Dec. 3, 2013, 11:35 a.m.
Message ID <2b49d4087af707302eb2.1386070548@stonewall.local>
Download mbox | patch
Permalink /patch/3203/
State Rejected
Headers show

Comments

Chris Jerdonek - Dec. 3, 2013, 11:35 a.m.
# HG changeset patch
# User Chris Jerdonek <chris.jerdonek@gmail.com>
# Date 1386070245 28800
#      Tue Dec 03 03:30:45 2013 -0800
# Node ID 2b49d4087af707302eb2acf23b2ec9cea17a4adc
# Parent  7eda5bb9ec8ff230d035164efde065d4f5ae92a2
parsers: add apiversion module constant to C implementation of parsers

This change adds a module constant called "apiversion" to the C implementation
of the parsers module.  Going forward, the presence of this constant can be
used to distinguish the C implementation of parsers from the pure Python
version (e.g. from within unit test modules).  The value is initially set to 1.

In addition, this value can be incremented as a way to signal changes in
the C API.  This will be useful in avoiding the need to recompile C code when
running tests across multiple changesets (e.g. using bisect).

This approach can be used, for example, when adding back the version mismatch
detection code that was backed out here: 96b2dd77c85d.  Using an approach
like a module constant is important in this case because otherwise, the
natural way to check whether the new API behavior is available for testing
is the same as the check that the new behavior is working, which would
invalidate using that way as a possible test.
Matt Mackall - Dec. 3, 2013, 9:43 p.m.
On Tue, 2013-12-03 at 03:35 -0800, Chris Jerdonek wrote:
> # HG changeset patch
> # User Chris Jerdonek <chris.jerdonek@gmail.com>
> # Date 1386070245 28800
> #      Tue Dec 03 03:30:45 2013 -0800
> # Node ID 2b49d4087af707302eb2acf23b2ec9cea17a4adc
> # Parent  7eda5bb9ec8ff230d035164efde065d4f5ae92a2
> parsers: add apiversion module constant to C implementation of parsers
> 
> This change adds a module constant called "apiversion" to the C implementation
> of the parsers module.  Going forward, the presence of this constant can be
> used to distinguish the C implementation of parsers from the pure Python
> version (e.g. from within unit test modules).  The value is initially set to 1.

This is still tightly coupled. Hint: bisect travels backwards and
forwards in history. I don't want to run 'make' if I'm not tracking down
an extension bug.

> In addition, this value can be incremented as a way to signal changes in
> the C API.

Also, this is a known recipe for failure, as already demonstrated in the
very bug this was created in response to. Human beings simply cannot be
entrusted to do this correctly, or more importantly, in a timely
fashion.

In C: add a new interface (do not change old interfaces)
In Python: check for existence of new interface
Chris Jerdonek - Dec. 3, 2013, 10:41 p.m.
On Tue, Dec 3, 2013 at 1:43 PM, Matt Mackall <mpm@selenic.com> wrote:
> On Tue, 2013-12-03 at 03:35 -0800, Chris Jerdonek wrote:
>> # HG changeset patch
>> # User Chris Jerdonek <chris.jerdonek@gmail.com>
>> # Date 1386070245 28800
>> #      Tue Dec 03 03:30:45 2013 -0800
>> # Node ID 2b49d4087af707302eb2acf23b2ec9cea17a4adc
>> # Parent  7eda5bb9ec8ff230d035164efde065d4f5ae92a2
>> parsers: add apiversion module constant to C implementation of parsers
>>
>> This change adds a module constant called "apiversion" to the C implementation
>> of the parsers module.  Going forward, the presence of this constant can be
>> used to distinguish the C implementation of parsers from the pure Python
>> version (e.g. from within unit test modules).  The value is initially set to 1.
>
> This is still tightly coupled. Hint: bisect travels backwards and
> forwards in history. I don't want to run 'make' if I'm not tracking down
> an extension bug.

You wouldn't need to.  The idea was to put certain C-specific tests
behind an if-block.  If the module constant didn't exist or the number
didn't match exactly (which could happen if the C wasn't recompiled),
the tests just wouldn't run.  Unless I'm missing something, that
pattern is forward and backwards-compatible.

>> In addition, this value can be incremented as a way to signal changes in
>> the C API.
>
> Also, this is a known recipe for failure, as already demonstrated in the
> very bug this was created in response to. Human beings simply cannot be
> entrusted to do this correctly, or more importantly, in a timely
> fashion.
>
> In C: add a new interface (do not change old interfaces)
> In Python: check for existence of new interface

The version-checking behavior doesn't require a change in the
interface, which is why I chose something general.  It just changes
what happens if you import the module when using the wrong version.
But I can change to something more meaningful.

--Chris

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -14,6 +14,8 @@ 
 
 #include "util.h"
 
+static int apiversion = 1;
+
 static int8_t hextable[256] = {
 	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
 	-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
@@ -1924,6 +1926,8 @@ 
 
 static void module_init(PyObject *mod)
 {
+	PyModule_AddIntConstant(mod, "apiversion", apiversion);
+
 	dirs_module_init(mod);
 
 	indexType.tp_new = PyType_GenericNew;
diff --git a/tests/test-parseindex2.py b/tests/test-parseindex2.py
--- a/tests/test-parseindex2.py
+++ b/tests/test-parseindex2.py
@@ -1,9 +1,22 @@ 
-"""This unit test tests parsers.parse_index2()."""
+"""This unit test primarily tests parsers.parse_index2().
+
+It also checks certain aspects of the parsers module as a whole.
+"""
 
 from mercurial import parsers
 from mercurial.node import nullid, nullrev
 import struct
 
+def usingc():
+    # Going forward we distinguish the C implementation of parsers from
+    # the pure Python version using the presence of the module-level
+    # constant apiversion.
+    try:
+        parsers.apiversion
+    except AttributeError:
+        return False
+    return True
+
 # original python implementation
 def gettype(q):
     return int(q & 0xFFFF)
@@ -95,7 +108,15 @@ 
     index, chunkcache = parsers.parse_index2(data, inline)
     return list(index), chunkcache
 
+def runctests():
+    """Run tests that should only be run against the C implementation."""
+    if not isinstance(parsers.apiversion, int):
+        print "parsers.apiversion not an int"
+
 def runtest() :
+    if usingc():
+        runctests()
+
     # Check that parse_index2() raises TypeError on bad arguments.
     try:
         parse_index2(0, True)