| 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
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
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)