From 165e33749c021a4cc9e8708944b5d908de319d31 Mon Sep 17 00:00:00 2001 From: "J. Lewis Muir" Date: Wed, 12 Sep 2018 16:54:46 -0500 Subject: [PATCH] Add win32 support Make adPython work on Windows. Change how adPython calls into the embedded Python to use PyGILState_Ensure() and PyGILState_Release() which is appropriate for calling from non-Python-created threads which is the case for adPython which is called from areaDetector-created threads. Without this change, adPython hangs on Windows. The Python calling idiom is documented in the following: * https://docs.python.org/2/c-api/init.html#non-python-created-threads * https://stackoverflow.com/a/4975906 * https://bugs.python.org/issue1720250#msg57619 Install the plugin scripts so that adPython can reference an installed pathname rather than a source pathname. Remove the automatic determination of the Python version, include pathnames, library pathnames, etc. by executing external commands from Make since it did not work on Windows. Replace that with explicit definitions in configure/CONFIG_SITE. --- adPythonApp/Makefile | 1 + adPythonApp/scripts/Makefile | 26 +++++++++ adPythonApp/src/Makefile | 44 +++++++++------- adPythonApp/src/adPythonPlugin.cpp | 85 +++++++++++++++++------------- adPythonApp/src/adPythonPlugin.h | 5 +- configure/CONFIG_PLUGINSCRIPTS | 2 + configure/CONFIG_SITE | 70 ++++++++++++++++++++++++ configure/Makefile | 2 + configure/RELEASE | 1 - 9 files changed, 177 insertions(+), 59 deletions(-) create mode 100644 adPythonApp/scripts/Makefile create mode 100644 configure/CONFIG_PLUGINSCRIPTS diff --git a/adPythonApp/Makefile b/adPythonApp/Makefile index 7b06cab..c97a0e9 100644 --- a/adPythonApp/Makefile +++ b/adPythonApp/Makefile @@ -6,5 +6,6 @@ DIRS := $(DIRS) $(filter-out $(DIRS), $(wildcard *db*)) DIRS := $(DIRS) $(filter-out $(DIRS), $(wildcard *Db*)) DIRS := $(DIRS) $(filter-out $(DIRS), $(wildcard *opi*)) DIRS := $(DIRS) $(filter-out $(DIRS), $(wildcard protocol)) +DIRS := $(DIRS) $(filter-out $(DIRS), $(wildcard scripts)) include $(TOP)/configure/RULES_DIRS diff --git a/adPythonApp/scripts/Makefile b/adPythonApp/scripts/Makefile new file mode 100644 index 0000000..871b85f --- /dev/null +++ b/adPythonApp/scripts/Makefile @@ -0,0 +1,26 @@ +TOP=../.. +include $(TOP)/configure/CONFIG +#---------------------------------------- +# ADD MACRO DEFINITIONS AFTER THIS LINE + +#---------------------------------------------------- +# Install scripts +PLUGINSCRIPTS += adPythonBarCode.py +PLUGINSCRIPTS += adPythonCircle.py +PLUGINSCRIPTS += adPythonCrystalDroplet.py +PLUGINSCRIPTS += adPythonCrystalFeatureMatch.py +PLUGINSCRIPTS += adPythonDataMatrix.py +PLUGINSCRIPTS += adPythonFocus.py +PLUGINSCRIPTS += adPythonGaussian2DFitter.py +PLUGINSCRIPTS += adPythonMitegen.py +PLUGINSCRIPTS += adPythonMorph.py +PLUGINSCRIPTS += adPythonMxSampleDetect.py +PLUGINSCRIPTS += adPythonPowerMean.py +PLUGINSCRIPTS += adPythonRotate.py +PLUGINSCRIPTS += adPythonTemplate.py +PLUGINSCRIPTS += adPythonTransfer.py +PLUGINSCRIPTS += transferclient.py + +include $(TOP)/configure/RULES +#---------------------------------------- +# ADD RULES AFTER THIS LINE diff --git a/adPythonApp/src/Makefile b/adPythonApp/src/Makefile index 3d1a015..e44a291 100644 --- a/adPythonApp/src/Makefile +++ b/adPythonApp/src/Makefile @@ -8,11 +8,18 @@ include $(TOP)/configure/CONFIG # This tells the compiler to ignore errors generated by EPICS includes. We need # this because the EPICS headers have non strict prototypes in places. +ifdef OS_CLASS +ifneq ($(OS_CLASS), WIN32) USR_CPPFLAGS += -isystem $(EPICS_BASE)/include +endif +endif # Nice and strict... +USR_CXXFLAGS += -Wall +ifdef OS_CLASS +ifneq ($(OS_CLASS), WIN32) USR_CXXFLAGS += -Werror -USR_CXXFLAGS += -Wall -Wextra +USR_CXXFLAGS += -Wextra USR_CXXFLAGS += -Wno-unused-parameter USR_CXXFLAGS += -Wno-missing-field-initializers USR_CXXFLAGS += -Wundef @@ -21,6 +28,8 @@ USR_CXXFLAGS += -Wcast-align USR_CXXFLAGS += -Wwrite-strings USR_CXXFLAGS += -Wredundant-decls USR_CXXFLAGS += -Wmissing-declarations +endif +endif LIBRARY_IOC += adPython @@ -30,31 +39,26 @@ DBD += adPythonPlugin.dbd # The following are compiled and added to the support library adPython_SRCS += adPythonPlugin.cpp -# Guess the python executable -ifneq ($(PYTHON_PREFIX),) - PYTHON = $(PYTHON_PREFIX)/bin/python -else - PYTHON = python -endif - -# Get the version string of python -PYTHON_VERSION = $(shell $(PYTHON) -c 'from distutils import sysconfig; \ - print sysconfig.get_config_var("VERSION")') - # link against python ifneq ($(PYTHON_PREFIX),) # user defined prefix, add it explicitly - USR_INCLUDES += -I$(PYTHON_PREFIX)/include/python$(PYTHON_VERSION) - python$(PYTHON_VERSION)_DIR = $(PYTHON_PREFIX)/lib - adPython_LIBS += python$(PYTHON_VERSION) - # add the numpy include path too, which is printed out when we run - # adPythonPlugin.py from the command line - USR_INCLUDES += -I$(shell $(PYTHON) ../adPythonPlugin.py) + USR_INCLUDES += -I$(PYTHON_INCLUDE) + USR_INCLUDES += -I$(NUMPY_INCLUDE) + python$(PYTHON_VERSION)_DIR = $(PYTHON_LIB) + LIB_LIBS += python$(PYTHON_VERSION) else # it's in a standard place - adPython_SYS_LIBS += python$(PYTHON_VERSION) + LIB_SYS_LIBS += python$(PYTHON_VERSION) endif -USR_CXXFLAGS += -DDATADIRS=\"$(shell cd ..; pwd):$(shell cd ../../scripts; pwd)\" +# add more libs +LIB_LIBS += NDPlugin + +# location of plugin scripts +USR_CXXFLAGS += -DDATADIRS=\"$(ADPYTHON_MODULE_PATH)\" + +# install plugin scripts +PLUGINSCRIPTS += adPythonOffline.py +PLUGINSCRIPTS += adPythonPlugin.py include $(TOP)/configure/RULES diff --git a/adPythonApp/src/adPythonPlugin.cpp b/adPythonApp/src/adPythonPlugin.cpp index ab8505f..07f85ac 100644 --- a/adPythonApp/src/adPythonPlugin.cpp +++ b/adPythonApp/src/adPythonPlugin.cpp @@ -8,7 +8,6 @@ #define NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION #include "numpy/ndarrayobject.h" #include -#include #include #include "NDArray.h" #include "adPythonPlugin.h" @@ -34,14 +33,16 @@ #define Bad(errString) NoGood(errString, BAD) #define Ugly(errString) NoGood(errString, UGLY) +// Used in setting the python module search path +#ifdef _WIN32 +#define PATH_LIST_SEPARATOR ";" +#else +#define PATH_LIST_SEPARATOR ":" +#endif + // Used in error printing const char *driverName = "adPythonPlugin"; -// This holds the threadState of the thread that initialised python -// We need it to get a handle on the interpreter so create a threadState -// object for each port -static PyThreadState *mainThreadState = NULL; - adPythonPlugin::adPythonPlugin(const char *portNameArg, const char *filename, const char *classname, int queueSize, int blockingCallbacks, const char *NDArrayPort, int NDArrayAddr, int maxBuffers, @@ -81,24 +82,24 @@ adPythonPlugin::adPythonPlugin(const char *portNameArg, const char *filename, */ void adPythonPlugin::initThreads() { - // First we tell python where to find adPythonPlugin.py and other scripts - char buffer[BIGBUFFER]; - snprintf(buffer, sizeof(buffer), "PYTHONPATH=%s", DATADIRS); - putenv(buffer); - - // Now we initialise python + PyGILState_STATE gstate; + + // Initialise python if (!Py_IsInitialized()) { PyEval_InitThreads(); Py_Initialize(); - - // Be sure to save thread state to release the GIL and give a handle - // on the interpreter to this and other ports - mainThreadState = PyEval_SaveThread(); + // Release the GIL + PyEval_SaveThread(); } - // Create a thread state just for us - this->threadState = PyThreadState_New(mainThreadState->interp); - PyEval_RestoreThread(this->threadState); + // Acquire the GIL and set up non-python-created thread access + gstate = PyGILState_Ensure(); + + // Tell python where to find adPythonPlugin.py and other scripts + char buffer[BIGBUFFER]; + snprintf(buffer, sizeof(buffer), "%s%s%s", Py_GetPath(), + PATH_LIST_SEPARATOR, DATADIRS); + PySys_SetPath(buffer); // Import our supporting library this->importAdPythonModule(); @@ -117,8 +118,8 @@ void adPythonPlugin::initThreads() this->updateParamList(1); } - // Release the GIL and finish - this->threadState = PyEval_SaveThread(); + // Release the GIL and tear down non-python-created thread access + PyGILState_Release(gstate); } /** Callback function that is called by the NDArray driver with new NDArray data @@ -130,6 +131,8 @@ void adPythonPlugin::initThreads() * Called with this->lock taken */ void adPythonPlugin::processCallbacks(NDArray *pArray) { + PyGILState_STATE gstate; + // First call the base class method NDPluginDriver::processCallbacks(pArray); @@ -143,8 +146,8 @@ void adPythonPlugin::processCallbacks(NDArray *pArray) { this->unlock(); epicsMutexLock(this->dictMutex); - // Make sure we're allowed to use the python API - PyEval_RestoreThread(this->threadState); + // Acquire the GIL and set up non-python-created thread access + gstate = PyGILState_Ensure(); this->lock(); // Store the time at the beginning of processing for profiling @@ -181,9 +184,11 @@ void adPythonPlugin::processCallbacks(NDArray *pArray) { setDoubleParam(adPythonTime, epicsTimeDiffInSeconds(&end, &start)*1000); callParamCallbacks(); - // release GIL and dict Mutex + // Unlock this->unlock(); - this->threadState = PyEval_SaveThread(); + // Release the GIL and tear down non-python-created thread access + PyGILState_Release(gstate); + // Release dict mutex epicsMutexUnlock(this->dictMutex); // Spit out the array @@ -209,14 +214,15 @@ asynStatus adPythonPlugin::writeInt32(asynUser *pasynUser, epicsInt32 value) { int param = pasynUser->reason; if (param == adPythonLoad || (this->nextParam && param >= adPythonUserParams[0])) { + PyGILState_STATE gstate; // We have to modify our python dict to match our param list // Note: to avoid deadlocks we should always take locks in order: // dictMutex, then GIL, then this->lock // so unlock here to preserve this order this->unlock(); epicsMutexLock(this->dictMutex); - // Make sure we're allowed to use the python API - PyEval_RestoreThread(this->threadState); + // Acquire the GIL and set up non-python-created thread access + gstate = PyGILState_Ensure(); // Now call the bast class to write the value to the param list this->lock(); status |= NDPluginDriver::writeInt32(pasynUser, value); @@ -230,8 +236,9 @@ asynStatus adPythonPlugin::writeInt32(asynUser *pasynUser, epicsInt32 value) { // our param lib has changed, so update the dict and reprocess status |= this->updateParamDict(); } - // release GIL and dict Mutex - this->threadState = PyEval_SaveThread(); + // Release the GIL and tear down non-python-created thread access + PyGILState_Release(gstate); + // Release dict mutex epicsMutexUnlock(this->dictMutex); } else { status |= NDPluginDriver::writeInt32(pasynUser, value); @@ -252,21 +259,23 @@ asynStatus adPythonPlugin::writeFloat64(asynUser *pasynUser, int status = asynSuccess; int param = pasynUser->reason; if (this->nextParam && param >= adPythonUserParams[0]) { + PyGILState_STATE gstate; // We have to modify our python dict to match our param list // Note: to avoid deadlocks we should always take locks in order: // dictMutex, then GIL, then this->lock // so unlock here to preserve this order this->unlock(); epicsMutexLock(this->dictMutex); - // Make sure we're allowed to use the python API - PyEval_RestoreThread(this->threadState); + // Acquire the GIL and set up non-python-created thread access + gstate = PyGILState_Ensure(); // Now call the bast class to write the value to the param list this->lock(); status |= NDPluginDriver::writeFloat64(pasynUser, value); // our param lib has changed, so update the dict and reprocess status |= this->updateParamDict(); - // release GIL and dict Mutex - this->threadState = PyEval_SaveThread(); + // Release the GIL and tear down non-python-created thread access + PyGILState_Release(gstate); + // Release dict mutex epicsMutexUnlock(this->dictMutex); } else { status = NDPluginDriver::writeFloat64(pasynUser, value); @@ -289,21 +298,23 @@ asynStatus adPythonPlugin::writeOctet(asynUser *pasynUser, const char *value, int status = asynSuccess; int param = pasynUser->reason; if (this->nextParam && param >= adPythonUserParams[0]) { + PyGILState_STATE gstate; // We have to modify our python dict to match our param list // Note: to avoid deadlocks we should always take locks in order: // dictMutex, then GIL, then this->lock // so unlock here to preserve this order this->unlock(); epicsMutexLock(this->dictMutex); - // Make sure we're allowed to use the python API - PyEval_RestoreThread(this->threadState); + // Acquire the GIL and set up non-python-created thread access + gstate = PyGILState_Ensure(); // Now call the bast class to write the value to the param list this->lock(); status |= NDPluginDriver::writeOctet(pasynUser, value, maxChars, nActual); // our param lib has changed, so update the dict and reprocess status |= this->updateParamDict(); - // release GIL and dict Mutex - this->threadState = PyEval_SaveThread(); + // Release the GIL and tear down non-python-created thread access + PyGILState_Release(gstate); + // Release dict mutex epicsMutexUnlock(this->dictMutex); } else { status |= NDPluginDriver::writeOctet(pasynUser, value, maxChars, nActual); diff --git a/adPythonApp/src/adPythonPlugin.h b/adPythonApp/src/adPythonPlugin.h index 2c1d0ce..e286e1f 100644 --- a/adPythonApp/src/adPythonPlugin.h +++ b/adPythonApp/src/adPythonPlugin.h @@ -6,6 +6,10 @@ #include #include +#if defined(_MSC_VER) && !defined(__func__) +#define __func__ __FUNCTION__ +#endif + // Max number of user parameters in a subclass #define NUSERPARAMS 100 @@ -58,7 +62,6 @@ class adPythonPlugin : public NDPluginDriver { NDAttributeList *pFileAttributes; int nextParam, pluginState; epicsMutexId dictMutex; - PyThreadState *threadState; }; #endif diff --git a/configure/CONFIG_PLUGINSCRIPTS b/configure/CONFIG_PLUGINSCRIPTS new file mode 100644 index 0000000..670b845 --- /dev/null +++ b/configure/CONFIG_PLUGINSCRIPTS @@ -0,0 +1,2 @@ +FILE_TYPE += PLUGINSCRIPTS +INSTALL_PLUGINSCRIPTS = $(INSTALL_LOCATION)/plugin-scripts diff --git a/configure/CONFIG_SITE b/configure/CONFIG_SITE index f30c124..6d8c7de 100644 --- a/configure/CONFIG_SITE +++ b/configure/CONFIG_SITE @@ -31,3 +31,73 @@ CROSS_COMPILER_TARGET_ARCHS = # You must rebuild in the iocBoot directory for this to # take effect. #IOCS_APPL_TOP = + +# Set this to the Python major-minor version string used in the +# installed Python filenames (e.g. "2.7" in +# "/usr/lib/python2.7"). This can vary by OS (e.g. for Python +# 2.7.15 on Windows, it is "27", but on a UNIX-like OS, it is +# "2.7"). +PYTHON_VERSION = 2.7 + +# Set this to the Python prefix (e.g. /opt/python-2.7.15). If +# not set, the system Python will be used. +PYTHON_PREFIX = $(TOOLS_BASE)/Python/2-7-3/prefix + +# Set this to the Python library directory. This is the +# directory containing the libpython$(PYTHON_VERSION) library. +# This can vary by OS (e.g. for Python 2.7.15 on Windows, the +# directory filename is "libs", but on a UNIX-like OS, it is +# "lib"). +# This is only used if PYTHON_PREFIX is not empty. +PYTHON_LIB = $(PYTHON_PREFIX)/lib + +# Set this to the Python include directory. This is the +# directory containing the Python C header files such as +# Python.h. This can vary by OS (e.g. for Python 2.7.15 on +# Windows, the relative pathname is "include", but on a +# UNIX-like OS, it is "include/python$(PYTHON_VERSION)"). +# This is only used if PYTHON_PREFIX is not empty. +PYTHON_INCLUDE = $(PYTHON_PREFIX)/include/python$(PYTHON_VERSION) + +# Set this to the location of the Python site-packages. This can +# vary by OS (e.g. for Python 2.7.15 on Windows, the relative +# pathname is "Lib/site-packages", but on a UNIX-like OS, it +# is "lib/python$(PYTHON_VERSION)/site-packages"). +# This is only used if PYTHON_PREFIX is not empty. +PYTHON_SITE_PACKAGES = $(PYTHON_PREFIX)/lib/python$(PYTHON_VERSION)/site-packages + +# Set this to the NumPy include directory. The directory can be +# determined by running +# python adPythonApp/src/adPythonPlugin.py +# where "python" runs the executable interpreter of the Python +# that will be embedded in adPython. +# This is only used if PYTHON_PREFIX is not empty. +NUMPY_INCLUDE = $(PYTHON_SITE_PACKAGES)/numpy/core/include + +# Set this to the delimiter character for path lists. If empty, +# it will be set automatically based on OS_CLASS. +PATH_LIST_SEPARATOR = +ifeq ($(PATH_LIST_SEPARATOR),) + ifeq ($(OS_CLASS), WIN32) + PATH_LIST_SEPARATOR = ; + else + PATH_LIST_SEPARATOR = : + endif +endif + +# This is automatically set to PYTHON_SITE_PACKAGES followed by +# PATH_LIST_SEPARATOR if PYTHON_PREFIX is not empty. If +# PYTHON_PREFIX is empty, this is automatically set to the +# empty string. +_PYTHON_SITE_PACKAGES_AND = +ifneq ($(PYTHON_PREFIX),) + _PYTHON_SITE_PACKAGES_AND = $(PYTHON_SITE_PACKAGES)$(PATH_LIST_SEPARATOR) +endif + +# Set this to the locations of adPython modules, adPython plugin +# scripts, and modules needed by those. This will be appended +# to the module search path of the embedded Python instance. +# This has the same format as the PYTHONPATH environment +# variable (i.e. a colon-separated list of paths, except on +# Windows where the list is instead semicolon-separated). +ADPYTHON_MODULE_PATH = $(_PYTHON_SITE_PACKAGES_AND)$(abspath $(INSTALL_LOCATION)/plugin-scripts) diff --git a/configure/Makefile b/configure/Makefile index 9254309..dd79bcc 100644 --- a/configure/Makefile +++ b/configure/Makefile @@ -2,6 +2,8 @@ TOP=.. include $(TOP)/configure/CONFIG +CFG += CONFIG_PLUGINSCRIPTS + TARGETS = $(CONFIG_TARGETS) CONFIGS += $(subst ../,,$(wildcard $(CONFIG_INSTALLS))) diff --git a/configure/RELEASE b/configure/RELEASE index 622362f..0bd218f 100644 --- a/configure/RELEASE +++ b/configure/RELEASE @@ -26,7 +26,6 @@ TOOLS_BASE=/dls_sw/prod/tools/RHEL6-x86_64 ASYN=$(SUPPORT)/asyn/4-26 ADCORE=$(SUPPORT)/ADCore/2-4dls8 -PYTHON_PREFIX=$(TOOLS_BASE)/Python/2-7-3/prefix # EPICS_BASE usually appears last so other apps can override stuff: EPICS_BASE=/dls_sw/epics/R3.14.12.3/base