On Mon, 27 Jun 2011, Mike Frysinger wrote:
> On Saturday, June 25, 2011 13:32:05 steve tell wrote:
>> --- /dev/null
>> +++ b/urjtag/bindings/python/Makefile.am
>> +build: chain.c
>> + -{ $(PYTHON) setup.py build && touch build; } || { $(RM) -r build; exit
>> 1;
> }
I cribbed that from another python-extension built with autotools.
> i dont think that should start with a "-". how about:
> set -e; \
> $(PYTHON) setup.py build; \
> touch build
Right that it shouldn't be ignored with a leading '-', but I'm not sure
what the possible issues are from not explicitly deleting build/ on a
setup failure.
>> --- /dev/null
>> +++ b/urjtag/bindings/python/chain.c
>>
>> +static PyObject *UrjtagError;
>
> i dont know anything about python bindings, but does this mean you can only
> have one instance ? what if people instantiate multiple chains ?
The thing pointed by UrjtagError is a python class, which can get
instantiated multiple times if needed.
>> +typedef struct
>> +{
>> + PyObject_HEAD urj_chain_t *urchain;
>> +} Chain;
>
> if the internal naming doesnt matter, i'd prefer something like "urj_py_xxx"
> rather than "Chain_xx" ...
Python tradition seems to be to name these functions after the class,
which traditional start with a capital letter. But since I named it
"urjtag.chain" instead of "Chain" somthing ought to change.
urj_py_chain_foo() is maximally descriptive, but is getting a little long.
I do expect to need python wrappings for other urjtag types at some point.
> you use this construct a lot (almost all funcs?). probably better to add a
> local func like:
good idea. maybe I can combine some of the checking into such a thing.
Another idiom that appears all the time is:
if (urj_tap_chain_connect (urc, drivername, cable_params) !=
URJ_STATUS_OK)
{
if (urj_error_get ())
{
PyErr_SetString (UrjtagError, urj_error_describe ());
urj_error_reset ();
}
else
{
PyErr_SetString (UrjtagError,
"unknown urjtag error in cable()");
}
return NULL;
The else clause is necessary because I found that sometimes the ujtag
routines return failure without calling urj_error_set.
Should I consider this a bug and eventually patch the core to set
an appropriate urj_error instead of working around? While chasing these
down, a stronger warning or even assert would be helpful, but that should
probably be configurable or eventually cleaned up.
>> --- a/urjtag/configure.ac
>> +++ b/urjtag/configure.ac
>>
>> +AC_ARG_ENABLE([python],
>> + [AS_HELP_STRING([--enable-python], [build python language bindings for
> liburjtag])],
>> + [BIND_PYTHON="yes"],[BIND_PYTHON="$enableval"])
>
> the 3rd/4th args are actually inverted here. the 3rd should be using
> $enableval while the 4th handles default values.
>
> the 4th should probably be BIND_PYTHON=detect, and then the following logic
> would be something like:
> if test "x$BIND_PYTHON" != "xno"; then
> ... detect python ...
> if python-is-too-old
> if BIND_PYTHON = yes
> AC_MSG_ERROR(...)
> else
> AC_MSG_WARN(...)
> fi
> fi
> fi
Just so I'm clear when testing, what is the desired logic for
various configure usages? I'm guessing that we want:
--enable-python - test for usable python, and fail configure if not found
--disable-python - don't test for python, and don't build python bindings
(default) - test for usable python, use it if found, else
print warnings and continue on without python bindings
thanks again for the thorough review!
Steve
------------------------------------------------------------------------------
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
